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

Fix phase order problem with throw helper blocks #97201

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

BruceForstall
Copy link
Member

Throw helper blocks are created in morph, then possibly removed if unnecessary in StackLevelSetter (under optimization). However, there was a case where StackLevelSetter removed an OVERFLOW throw helper block after optimization proved it unnecessary because of a constant zero dividend, but between StackLevelSetter and codegen, LSRA introduced a RELOAD node above the constant zero that GenTree::CanDivOrModPossiblyOverflow() didn't understand, thus causing it to think that overflow was possible. Codegen looked for the OVERFLOW throw helper block and couldn't find it.

There are multiple fixes here, somewhat "defense in depth":

  • If StackLevelSetter::SetThrowHelperBlocks() determines a node can't throw divide-by-zero or ArithmeticException (overflow), it marks the node GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW, respectively. This is what morph does earlier in compilation.
  • genMarkLabelsForCodegen() does not mark throw helper blocks where acdUsed is false, to avoid marking deleted blocks.
  • More asserts are added that acdUsed is true when codegen goes to generate a branch to a throw helper.
  • GenTree::OperExceptions / CanDivOrModPossiblyOverflow are changed to skip COPY/RELOAD nodes.

Fixes #96224

Throw helper blocks are created in morph, then possibly removed if
unnecessary in StackLevelSetter (under optimization). However, there
was a case where StackLevelSetter removed an OVERFLOW throw helper
block after optimization proved it unnecessary because of a constant
zero dividend, but between StackLevelSetter and codegen, LSRA introduced
a RELOAD node above the constant zero that `GenTree::CanDivOrModPossiblyOverflow()`
didn't understand, thus causing it to think that overflow was possible.
Codegen looked for the OVERFLOW throw helper block and couldn't find it.

There are multiple fixes here, somewhat "defense in depth":
- If `StackLevelSetter::SetThrowHelperBlocks()` determines a node can't throw
divide-by-zero or ArithmeticException (overflow), it marks the node
GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW, respectively. This is what
morph does earlier in compilation.
- `genMarkLabelsForCodegen()` does not mark throw helper blocks where `acdUsed`
is false, to avoid marking deleted blocks.
- More asserts are added that `acdUsed` is true when codegen goes to generate
a branch to a throw helper.
- `GenTree::OperExceptions` / `CanDivOrModPossiblyOverflow` are changed to skip
COPY/RELOAD nodes.

Fixes dotnet#96224
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 19, 2024
@ghost ghost assigned BruceForstall Jan 19, 2024
@ghost
Copy link

ghost commented Jan 19, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Throw helper blocks are created in morph, then possibly removed if unnecessary in StackLevelSetter (under optimization). However, there was a case where StackLevelSetter removed an OVERFLOW throw helper block after optimization proved it unnecessary because of a constant zero dividend, but between StackLevelSetter and codegen, LSRA introduced a RELOAD node above the constant zero that GenTree::CanDivOrModPossiblyOverflow() didn't understand, thus causing it to think that overflow was possible. Codegen looked for the OVERFLOW throw helper block and couldn't find it.

There are multiple fixes here, somewhat "defense in depth":

  • If StackLevelSetter::SetThrowHelperBlocks() determines a node can't throw divide-by-zero or ArithmeticException (overflow), it marks the node GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW, respectively. This is what morph does earlier in compilation.
  • genMarkLabelsForCodegen() does not mark throw helper blocks where acdUsed is false, to avoid marking deleted blocks.
  • More asserts are added that acdUsed is true when codegen goes to generate a branch to a throw helper.
  • GenTree::OperExceptions / CanDivOrModPossiblyOverflow are changed to skip COPY/RELOAD nodes.

Fixes #96224

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@BruceForstall
Copy link
Member Author

This is presumably fallout from #95379

@BruceForstall
Copy link
Member Author

@AndyAyersMS PTAL
cc @dotnet/jit-contrib

@BruceForstall BruceForstall merged commit 0f07c39 into dotnet:main Jan 19, 2024
127 of 129 checks passed
@BruceForstall BruceForstall deleted the FixTgtIGAssert branch January 19, 2024 19:03
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
Throw helper blocks are created in morph, then possibly removed if
unnecessary in StackLevelSetter (under optimization). However, there
was a case where StackLevelSetter removed an OVERFLOW throw helper
block after optimization proved it unnecessary because of a constant
zero dividend, but between StackLevelSetter and codegen, LSRA introduced
a RELOAD node above the constant zero that `GenTree::CanDivOrModPossiblyOverflow()`
didn't understand, thus causing it to think that overflow was possible.
Codegen looked for the OVERFLOW throw helper block and couldn't find it.

There are multiple fixes here, somewhat "defense in depth":
- If `StackLevelSetter::SetThrowHelperBlocks()` determines a node can't throw
divide-by-zero or ArithmeticException (overflow), it marks the node
GTF_DIV_MOD_NO_BY_ZERO / GTF_DIV_MOD_NO_OVERFLOW, respectively. This is what
morph does earlier in compilation.
- `genMarkLabelsForCodegen()` does not mark throw helper blocks where `acdUsed`
is false, to avoid marking deleted blocks.
- More asserts are added that `acdUsed` is true when codegen goes to generate
a branch to a throw helper.
- `GenTree::OperExceptions` / `CanDivOrModPossiblyOverflow` are changed to skip
COPY/RELOAD nodes.

Fixes dotnet#96224
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fuzzlyn] Assertion failed 'tgtIG' during 'Generate code'
2 participants