This is an archive of the discontinued LLVM Phabricator instance.

[NFC][LoopIdiom] Add more test case to runtime-determined memset size
ClosedPublic

Authored by eopXD on Aug 21 2021, 3:10 AM.

Details

Summary

This patch supplements missing test case for D107353.

  • Fix wrong descriptions in 64-bit mode test case
  • Added testcase under 32-bit mode

Diff Detail

Event Timeline

eopXD requested review of this revision.Aug 21 2021, 3:10 AM
eopXD created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2021, 3:10 AM
eopXD edited the summary of this revision. (Show Details)Aug 21 2021, 3:29 AM
eopXD added a comment.Aug 25 2021, 8:56 PM

gentle ping.

I'm not sure this gives us anything.

eopXD added a comment.Aug 26 2021, 1:15 AM

I'm not sure this gives us anything.

As @bmahjour was commenting in D108112, he questioned on how the patch will operate on 32-bit mode.
I think adding test cases of 32-bit mode will answer his doubt.

bmahjour added inline comments.Sep 13 2021, 11:36 AM
llvm/test/Transforms/LoopIdiom/memset-runtime-64bit.ll
18

For better coverage, we should also test with long long loop bounds in 32-bit mode and int bounds in 64-bit mode.

llvm/test/Transforms/LoopIdiom/memset-runtime.ll
1

What's the dependency between this patch and D108112? Should we expect to see the updates made to memset-runtime.ll under that patch to apply on top of this change?

eopXD updated this revision to Diff 380034.Oct 15 2021, 9:18 AM

Address comments - add test cases.

eopXD updated this revision to Diff 380199.EditedOct 16 2021, 10:17 AM
eopXD marked an inline comment as done.

Adjust tescases to show its relationship with D108112.

@bmahjour

What's the dependency between this patch and D108112?

There was only 64-bit test cases when I was doing D108112.
Then in the comments you questioned on how would LIR perform under 32bit mode.
So I opened this patch to split original test file with only 64-bit mode test case into 2 files that contains 32bit mode test case and 64bit mode test case.
You can see the optimization of LIR @NestedFor32 and @NestedFor64 are different under 32bit mode and 64bit mode.
It is because the SCEV will be transformed when LIR deals with the inner-loop and lets the SCEV not be recognize when LIR goes through the outer-loop.
In D108112, the SCEV rewriter will rewrite the SCEV to a form LIR can recognize and let the outer-loop also be optimized.

Should we expect to see the updates made to memset-runtime.ll under that patch to apply on top of this change?

Yes. D108112 should modify upon the test case this patch provides.
The checks of @NestedFor32 and @NestedFor64 will change in D108112 because of the SCEV rewriter.

eopXD marked an inline comment as done.Oct 16 2021, 10:17 AM
bmahjour added inline comments.Oct 18 2021, 7:41 AM
llvm/test/Transforms/LoopIdiom/memset-runtime-32bit.ll
100

match the order of arguments with the actual function.

177

The tests from here look very similar to the ones in memset-runtime-64bit.ll. I'd suggest using int i instead of long long i (or add at least one test where the loop upper bound is 8 bytes and the induction variable is 4 bytes).

269

match order of arguments with the actual function

eopXD updated this revision to Diff 381004.Oct 20 2021, 9:46 AM

Address comments.

  • match argument to actual function
  • have 4-byte induction variables in memset-runtime-32bit.ll with 8-byte upper bound.
bmahjour accepted this revision.Oct 20 2021, 12:41 PM
This revision is now accepted and ready to land.Oct 20 2021, 12:41 PM