Improve handling aborting instructions in IR. #2051
Assignees
Labels
Comments
otrho
added
enhancement
otrho
changed the title
7 tasks
IGI-111
pushed a commit
that referenced
this issue
Jan 29, 2024
## Description This PR refactors how `Value` is used during IR conversion. The PR introduces `TerminatorValue` which forces the converter to handle diverging expressions properly. There were checks for divergence scattered around the code before, but it's now been made more systematic and thus more robust. Additionally, this change allows for early returns from some functions. For instance, if the condition of a `while` diverges, then we don't need to generate code for the body of the loop. In fact, if an expression does diverge then the algorithm assumes that a terminator value (e.g., a `branch` or a `return`) is returned, but this was not always the case, and led to a number of spurious checks that probably didn't quite work. The heavy lifting is done by the macro `return_on_termination_or_extract`, which acts as destructor of `TerminatorValue`s. Most changes are straightforward, but do review the functions `compile_lazy_op`, `compile_fn_call`, `compile_if`, `compile_while_loop`, `compile_var_decl` and `compile_array_expr` extra carefully, as they contain changes to the conversion algorithm. Mostly the changes are small, but they need an extra pair of eyes. In a few cases the algorithm was and still is a bit convoluted, but that's because the typechecker unhelpfully assigns `Unknown` instead of `Never` to diverging expressions, so the first sub-expression needs to be compiled and divergence checked before we can generate IR for anything that relies on the type of the expression. Finally, I have taken the liberty of removing `Value::is_diverging()` and using its alias `Value::is_terminator()` everywhere. The concept of divergence lives in the source language, whereas the concept of terminators lives in the IR, so `is_terminator` seems to be more appropriate at this stage of the compilation. Fixes #2051. Partial fix of #5438 (the call to `deterministically_aborts()` is removed). ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Igor Rončević <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#2029 addresses problems we had with
returnstatements inifexpressions but it exposes a general problem IRgen has with control flow which aborts, and it may pop up again withreturninwhileloops and/or when we introducecontinueandbreak.The overall problem is IR blocks must be well formed -- they should have a single terminator as the last instruction and if it's a branch then the
phimust be correct and properly typed. But having an aborting instruction in a block could accidentally introduce dead instructions afterwards (in an effort to correctly type check) or could create dead empty blocks, e.g., the 'current' block is terminated by areturnand so a new 'current' block is created. (We must at all times have a current block while adding instructions to a function.)Or there may be other problems in the future (hrmm, one day there will be a
revertintrinsic and presumably an accompanying instruction). A general explicit solution or convention around aborting instructions needs to be introduced to the IRgen process.The text was updated successfully, but these errors were encountered: