This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Add tests for checking whether `div`/`rem` is safe to speculate; NFC
ClosedPublic

Authored by goldstein.w.n on Apr 27 2023, 11:22 PM.

Diff Detail

Unit TestsFailed

Event Timeline

goldstein.w.n created this revision.Apr 27 2023, 11:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2023, 11:22 PM
goldstein.w.n requested review of this revision.Apr 27 2023, 11:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2023, 11:23 PM
nikic added a comment.Apr 28 2023, 1:14 AM

The test cases would probably be simpler if this were using LICM.

llvm/test/Analysis/ValueTracking/speculate-div.ll
2 ↗(On Diff #517806)

Should have mentioned this earlier, but please don't put tests into llvm/test/Analysis/ValueTracking, unless they use special analysis printers. Instead put them into the pass being tested, here llvm/test/Transforms/SimplifyCFG.

7 ↗(On Diff #517806)

The function attributes shouldn't be needed.

Move tests + remove unneeded attrs

goldstein.w.n marked 2 inline comments as done.Apr 29 2023, 9:24 AM

What do you think about this test structure? https://llvm.godbolt.org/z/nMxMGoz1r Seems clearer to me than going through speculative hoisting.

goldstein.w.n added a comment.EditedApr 29 2023, 11:23 AM

What do you think about this test structure? https://llvm.godbolt.org/z/nMxMGoz1r Seems clearer to me than going through speculative hoisting.

Sure, where would that test belong? simplifycfg still?
Edit: putting in Transform/LICM.

Simplify tests + move to LICM

nikic accepted this revision.Apr 29 2023, 12:07 PM

LGTM

This revision is now accepted and ready to land.Apr 29 2023, 12:07 PM