Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cgirgen: fix translation of defs #954

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Oct 11, 2023

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 defs

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

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
@zerbina zerbina added refactor Implementation refactor compiler/backend Related to backend system of the compiler labels Oct 11, 2023
@zerbina zerbina added this to the MIR phase milestone Oct 11, 2023
@saem
Copy link
Collaborator

saem commented Oct 11, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


@chore-runner chore-runner bot added this pull request to the merge queue Oct 11, 2023
Merged via the queue into nim-works:devel with commit e62b836 Oct 11, 2023
18 checks passed
@zerbina zerbina deleted the cgirgen-fix-def-translation branch October 12, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/backend Related to backend system of the compiler refactor Implementation refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants