min reproducer of brilck missed error

[Pat-Lafon]

Here is a minimal reproducible example that I believe is both a type error and missed by brilck. This was found in #459 and was introduced in #448. Note that I have made a slight change from the original error to make the minimal example not dead code, this still exhibits a missing check for when a return type is set, a value is expected to be returned from the function call, but there is no return instruction.

[sampsyo]

Thanks for putting this together! This is a deceptively interesting one, because enforcing the absence of "fall-through returns" in non-void functions is hard-ish to do in general. For example:

@foo(): int {
  one: int = const 1;
  jmp .lbl;
  ret one;
.lbl:
}

That program has an appropriate ret but is still going to fail at run time in the same way. But this program is fine:

@foo(): int {
  one: int = const 1;
  ret one;
  jmp .lbl;
.lbl:
}

A precise check for this error would have some trouble distinguishing these two cases.

To summarize:

1. brilck could emit an error when there are zero appropriate rets in a non-void function, because this is guaranteed to be an error. But requiring at least one such ret is insufficient to eliminate this class of error at run time, and perhaps even the consequent crashes in brillvm. But it's probably worth doing anyway?
2. We could require non-void functions to have a ret as their final instruction. That seems like a pretty awkward restriction.
3. A more sophisticated check could analyze the CFG to check that, if the function does not end in ret, then this final basic block is unreachable. I think that would be as precise as is reasonable to expect? But it would be weird because brilck currently does not do any CFG analysis; everything is instruction-level.

Anyway, I'll add a check for item 1 now. If you agree that item 3 is worth pursuing, maybe we should just file an issue for exploring that enhancement to brilck?

[sampsyo]

I folded your test into the brilck feature PR, #461! Thank you!

[Pat-Lafon]

Thanks for putting this together! This is a deceptively interesting one, because enforcing the absence of "fall-through returns" in non-void functions is hard-ish to do in general. For example:

@foo(): int {
  one: int = const 1;
  jmp .lbl;
  ret one;
.lbl:
}

That program has an appropriate ret but is still going to fail at run time in the same way. But this program is fine:

@foo(): int {
  one: int = const 1;
  ret one;
  jmp .lbl;
.lbl:
}

A precise check for this error would have some trouble distinguishing these two cases.

To summarize:

1. brilck could emit an error when there are _zero_ appropriate `ret`s in a non-void function, because this is _guaranteed_ to be an error. But requiring at least one such `ret` is insufficient to eliminate this class of error at run time, and perhaps even the consequent crashes in brillvm. But it's probably worth doing anyway?

2. We could require non-void functions to have a `ret` as their final instruction. That seems like a pretty awkward restriction.

3. A more sophisticated check could analyze the CFG to check that, if the function does not end in `ret`, then this final basic block is unreachable. I think that would be as precise as is reasonable to expect? But it would be weird because brilck currently does not do any CFG analysis; everything is instruction-level.

Anyway, I'll add a check for item 1 now. If you agree that item 3 is worth pursuing, maybe we should just file an issue for exploring that enhancement to brilck?

Ah this was harder than I expected. I implemented 1 as well for brilirs. brillvm previously had similar "well-formedness" restrictions on phi-nodes which was stronger than what Bril requires, this sounds like a similar situation(and hopefully an unlikely one to hit or I can revisit this as need be).

For 3, a precise enough analysis seems challenging. Maybe here is another example that doesn't rely on deadcode that should be allowed? There could be a style-driven "no obviously dead code" in the benchmark suite check but I'm not sure that it is worth pursuing.

@foo(): bool {
  cond: bool = const true;
  br cond .lbl .lbl2;
.lbl:
  ret cond;
.lbl2:
}

@main() {
    cond: bool = call @foo;
    print cond;
}

[sampsyo]

Yeah, that's a good point. I think it's not terribly clear what the static counterpart to the dynamic requirement ("non-void functions must return something") ought to be!