Skip to content

codegen: remove the cnkChckRange node kinds #842

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

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 14, 2023

Summary

Replace the cnkChckRange node kinds with a magic call that is already
inserted during MIR synthesis (mirgen) and remove all decision making
regarding range-checks from the code generators.

This is a step towards moving decision making regarding run-time checks
fully into late semantic analysis. In addition, this fixes destructors
not being run for scopes where the only escaping unstructured control-
flow is due to range checks.

Details

A new internal magic is added for representing conversions-with-range-
checks during the MIR and code generation phase: mChckRange. mirgen
translates nkChckRange, nkChckRange64, and nkChckRangeF to the
new magic, but only if its not a to-unsigned conversion and range checks
are enabled for the current owner.

For now, these guards mirror those previously used in the code
generators, but the direction is to move to a correct-by-construction
approach and only inject nkChckRange nodes where range checks should
be performed.

The mChckRange magic is included in the magicsThatCanRaise set, and
the duplicate set of the same name is removed from mirexec. As a
consequence, the exceptional control-flow arising from failed range
checks is now picked up on by MIR-based data- and control-flow analysis.
However, since the set is constant, this also means that range-check
calls will be treated as raising even when panics are enabled. This is a
known problem with magics that needs to be solved at one point.

Multiple things are now obsolete and thus removed:

  • the cnkChckRange node kinds
  • the cgirgen logic for producing cnkChckRange nodes (which was
    originally duplicated from the logic in transf.transformConv)
  • the code generator handling for cnkChckRange. Parts of it are
    re-used for implementing mChckRange support

Two additional problems are fixed as a side-effect of the changes:

  • the VM code generator now respects the rangeChecks option in
    the same way as the other code generator
  • disabled range checks (either via the rangeChecks option or
    because the target type is unsigned) are not treated as no-op
    by the JS code generator, but instead use unchecked conversions
    (the tdollars.nim test is adjusted accordingly), in line with
    the other code generators

Summary
=======

Replace the `cnkChckRange` node kinds with a magic call that is already
inserted during MIR synthesis (`mirgen`) and remove all decision making
regarding range-checks from the code generators.

This is a step towards moving decision making regarding run-time checks
fully into late semantic analysis. In addition, this fixes destructors
not being run for scopes where the only escaping unstructured control-
flow is due to range checks.

Details
=======

A new internal magic is added for representing conversions-with-range-
checks during the MIR and code generation phase: `mChckRange`. `mirgen`
translates `nkChckRange`, `nkChckRange64`, and `nkChckRangeF` to the
new magic, but only if its not a to-unsigned conversion and range checks
are enabled for the current owner.

For now, these guards mirror those previously used in the code
generators, but the direction is to move to a correct-by-construction
approach and only inject `nkChckRange` nodes where range checks should
be performed. As a side-effect the VM code generator now respects the
`rangeChecks` option.

The `mChckRange` magic is included in the `magicsThatCanRaise` set, and
the duplicate set of the same name is removed from `mirexec`. As a
consequence, the exceptional control-flow arising from failed range
checks is now picked up on by MIR-based data- and control-flow analysis.
However, since the set is constant, this also means that range-check
calls will be treated as raising even when panics are enabled. This is a
known problem with magics that needs to be solved at one point.

Multiple things are now obsolete and thus removed:
- the `cnkChckRange` node kinds
- the `cgirgen` logic for producing `cnkChckRange` nodes (which was
  originally duplicated from the logic in `transf.transformConv`)
- the code generator handling for `cnkChckRange`. Parts of it are
  re-used for implementing `mChckRange` support
The behaviour with the JS backend is now consistent with that of the
others.
@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Aug 14, 2023
@zerbina zerbina added this to the MIR phase milestone Aug 14, 2023
@zerbina zerbina linked an issue Aug 14, 2023 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the greater regularity in handling of exceptional control is particularly cool.

@saem
Copy link
Collaborator

saem commented Aug 14, 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 Aug 14, 2023
Merged via the queue into nim-works:devel with commit 95b3530 Aug 14, 2023
@zerbina zerbina deleted the mirgen-driven-range-checks branch August 21, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sem-driven run-time checks
2 participants