This is an archive of the discontinued LLVM Phabricator instance.

[LSR] Precommit test for D132443
ClosedPublic

Authored by eopXD on Aug 23 2022, 1:44 AM.

Details

Summary

Pre-commit test for D132443

Diff Detail

Event Timeline

eopXD created this revision.Aug 23 2022, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 1:44 AM
eopXD requested review of this revision.Aug 23 2022, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 1:44 AM
eopXD updated this revision to Diff 454748.Aug 23 2022, 2:02 AM

Simplify the testcase even more.

eopXD edited the summary of this revision. (Show Details)Aug 23 2022, 2:03 AM
eopXD edited the summary of this revision. (Show Details)
eopXD added reviewers: Restricted Project, Whitney, Meinersbur, bmahjour, monkchiang, congzhe.Aug 23 2022, 9:23 AM
eopXD updated this revision to Diff 455487.Aug 25 2022, 12:33 AM

Update comments in testcases.

craig.topper added inline comments.
llvm/test/Transforms/LoopStrengthReduce/lsr-fold-iv-complicate-add-rec.ll
2 ↗(On Diff #455487)

Move the RUN line above this description

llvm/test/Transforms/LoopStrengthReduce/lsr-fold-iv-const-tripcount.ll
2 ↗(On Diff #455487)

Move the RUN line above this description

llvm/test/Transforms/LoopStrengthReduce/lsr-fold-iv-runtime-tripcount.ll
2 ↗(On Diff #455487)

Same

fhahn added inline comments.Aug 26 2022, 10:19 AM
llvm/test/Transforms/LoopStrengthReduce/lsr-fold-iv-runtime-tripcount.ll
84 ↗(On Diff #455487)

is all those here really needed?

93 ↗(On Diff #455487)

is all this here really needed?

112 ↗(On Diff #455487)

is all those here really needed?

eopXD updated this revision to Diff 456124.Aug 27 2022, 9:18 AM

Simplify the testcase. Move RUN to above.

eopXD marked 6 inline comments as done.Aug 27 2022, 9:21 AM
fhahn added a comment.Aug 30 2022, 3:08 AM

Thanks for the update! Given hat all those tests seem to be slight variations of each other, should they be moved in the same file, with a comment highlighting the differences, which are mostly the trip count and/or the complexity of the addrec

llvm/test/Transforms/LoopStrengthReduce/lsr-fold-iv-complicate-add-rec.ll
36 ↗(On Diff #456124)

nit: easier to right if the exit block is at the end.

llvm/test/Transforms/LoopStrengthReduce/lsr-fold-iv-runtime-tripcount.ll
45 ↗(On Diff #456124)

is this needed? Could we not have a simple variant of /lsr-fold-iv-complicate-add-rec.ll where the incoming value is something like [ %N, %entry ] instead of [ 379, %entry ]?

eopXD updated this revision to Diff 456611.Aug 30 2022, 4:23 AM

Address comments from @fhahn

  • Merge 3 testcases into a single file
  • Simplify testcase for runtime tripcount
eopXD marked 2 inline comments as done.Aug 30 2022, 4:24 AM
eopXD updated this revision to Diff 456824.Aug 30 2022, 5:42 PM

Rename testcase.

eopXD added a comment.Sep 4 2022, 1:29 AM

Gentle ping, thank you.

fhahn added a comment.Sep 5 2022, 12:30 AM

Gentle ping, thank you.

as mentioned in D132443, it seems like more negative test coverage would be good.

llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold.ll
59

Nit: looks like a type ->ruin -> run :)

eopXD updated this revision to Diff 458122.Sep 6 2022, 2:17 AM
eopXD marked an inline comment as done.

Fix minor type-o.

This revision is now accepted and ready to land.Sep 18 2022, 7:43 PM
This revision was landed with ongoing or failed builds.Sep 19 2022, 7:07 PM
This revision was automatically updated to reflect the committed changes.