This is an archive of the discontinued LLVM Phabricator instance.

[ConstantFold] Remove unnecessary bounded index restriction
ClosedPublic

Authored by nikic on Jan 4 2022, 2:59 AM.

Details

Summary

The fold for merging a GEP of GEP into a single GEP currently bails if doing so would result in notional overindexing. The justification given in the comment above this check is dangerously incorrect: GEPs with notional overindexing are perfectly fine, and if some code treats them incorrectly, then that code is broken, not the GEP. Such a GEP might legally appear in source IR, so only preventing its creation cannot be sufficient. (The constant folder also ends up canonicalizing the GEP to remove the notional overindexing, but that's neither here nor there.)

This check dates back to https://github.com/llvm/llvm-project/commit/bd4fef4a8939db18f39b108e19097b25e2c7c47a, and as far as I can tell the original issue this was trying to patch around has since been resolved.

Diff Detail

Event Timeline

nikic created this revision.Jan 4 2022, 2:59 AM
nikic requested review of this revision.Jan 4 2022, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 2:59 AM
nikic updated this revision to Diff 397261.Jan 4 2022, 4:13 AM

Update some clang tests. Will update the rest if this is generally fine.

nlopes accepted this revision.Jan 4 2022, 4:20 AM

LGTM. I agree there's no issue in combining OOB geps.
The code seems to handle well the mix of inbounds and non-bounds geps, by dropping inbounds.

I glanced through the OMP test changes, as they looked scary, but they seem fine. They produce a +1 pointer, which is ok.

This revision is now accepted and ready to land.Jan 4 2022, 4:20 AM
This revision was landed with ongoing or failed builds.Jan 4 2022, 6:23 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2022, 6:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript