Skip to content

[SCEV] Stop blindly propagating inbounds from GEPs to SCEV nodes - #9116

Open
Natalie Chouinard (sudonatalie) wants to merge 3 commits into
microsoft:mainfrom
sudonatalie:b-584269341
Open

Natalie Chouinard (sudonatalie) wants to merge 3 commits into
microsoft:mainfrom
sudonatalie:b-584269341

Conversation

@sudonatalie

Copy link
Copy Markdown
Collaborator

ScalarEvolution::getGEPExpr previously converted a GEP instruction's inbounds flag into SCEV::FlagNSW on the index-by-size multiplication and base addition. When a GEP is guarded by control flow, its inbounds property may not hold across the entire loop, and attaching no-wrap flags to the resulting recurrence can pollute a globally uniqued SCEVAddRecExpr shared with another value (such as the loop's induction variable), causing IndVarSimplify to fold away live loop exit checks.

This resolves the PR23527 FIXME in ScalarEvolution::getGEPExpr, matching createSCEV's handling of Add and Mul and backporting the fix from: llvm/llvm-project@2ca8a3f (PR23527, D109789)

ScalarEvolution::getGEPExpr previously converted a GEP instruction's inbounds flag into SCEV::FlagNSW on the index-by-size multiplication and base addition. When a GEP is guarded by control flow, its inbounds property may not hold across the entire loop, and attaching no-wrap flags to the resulting recurrence can pollute a globally uniqued SCEVAddRecExpr shared with another value (such as the loop's induction variable), causing IndVarSimplify to fold away live loop exit checks.

This resolves the PR23527 FIXME in ScalarEvolution::getGEPExpr, matching createSCEV's handling of Add and Mul and backporting the fix from:
llvm/llvm-project@2ca8a3f (PR23527, D109789)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The fix for a user-visible miscompile that can remove live loop-exit checks lacks the release-note entry required for important compiler bug fixes.

1 open issue

Review effort: Balanced

Comment thread lib/Analysis/ScalarEvolution.cpp Resolved

@llvm-beanz Chris B (llvm-beanz) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Couple small nits.

Comment thread lib/Analysis/ScalarEvolution.cpp Resolved
Comment thread lib/Analysis/ScalarEvolution.cpp Resolved
Comment thread lib/Analysis/ScalarEvolution.cpp Resolved
Co-authored-by: Chris B <beanz@abolishcrlf.org>
Copilot AI balanced review requested due to automatic review settings November 12, 2026 18:55
Co-authored-by: Chris B <beanz@abolishcrlf.org>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

There are no active review issues for the ScalarEvolution no-wrap flag backport and its regression test.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the ScalarEvolution loop-exit miscompile fix.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 12, 2026 19:08
@sudonatalie

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the ScalarEvolution no-wrap propagation fix and its accompanying regression test.

No open issues 🎉

Review effort: Balanced

@sudonatalie

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

4 participants