From e62b836c8d062e8236a729f7fae09972ffb3c2a3 Mon Sep 17 00:00:00 2001 From: zerbina <100542850+zerbina@users.noreply.github.com> Date: Thu, 12 Oct 2023 00:40:25 +0200 Subject: [PATCH] cgirgen: fix translation of `def`s (#954) ## Summary Properly translate local definitions from the MIR to the `CgNode` IR. While the previous implementation didn't produce the wrong results (so far), it was based on incorrect assumptions. ## Details * add `tbBody` and use it for translating the body of control-flow constructs (`mnkIf`, `mnkBlock`, etc.) * replace the `inArgBlock` counter with the `inUnscoped` flag * remove the `tbStmt` overload that's now only used in a single place * instead of storing `cnkDef` nodes in the `defs` list, store the information required to produce the nodes; this is more efficient, as it means less work during clean-up (the contents of the `defs` seq don't require destruction) ### Translation of `def`s A `def` for a local/global in the MIR means "locations starts to exist and is accessible from all connected basic blocks within the same scope (`mnkScope`) to which the only control-flow path also visits the def". The `CgNode` IR doesn't have an explicit notion of "scope" as the MIR does, and placing a `cnkDef` in some control-flow- related construct (e.g., `cnkIf`) can produce the wrong code when the construct doesn't open a new scope itself (i.e., its body is not wrapped in an `mnkScope`). Previously, translating a `mnkDef` to an assignment, with the `cnkDef` placed at the start of the enclosing scope (this is also referred to as lifting the def), was performed for all defs within an argument block. However, argument blocks are wholly unrelated to the problem described above, and the associated comment was also wrong: a `cnkStmtList` doesn't open a scope. Due to a lot of code being located in some argument block, this incorrect implementation still produced the correct results so far, but changes to how MIR code is generated or how some of the MIR passes operate could break this at any time. The correct thing to look for is whether a definition is inside a control-flow constructs (`if`, `block`, etc.). If it is, and there's no `mnkScope` in-between, the definition needs to be lifted into the closest surrounding scope. An alternative approach would be introducing a `cnkScope` node for the `CgNode` IR and then leaving the work to the code generators. The reason for not going that route is that keeping the lifting logic in `cgirgen` for now makes it easier to change later on. ### Future work * only locals that are used outside of their enclosing control-flow construct need to be lifted; an analysis pass could detect whether this is the case * now that the semantics are clearer, the lifting of defs in the destructor injection pass can be removed