This is an archive of the discontinued LLVM Phabricator instance.

[NFCI] [LoopIdiom] Let processLoopStridedStore take StoreSize as SCEV instead of unsigned
ClosedPublic

Authored by eopXD on Jun 19 2021, 7:26 AM.

Details

Summary

Letting it take SCEV allows further modification on the function to optimize
if the StoreSize / Stride is runtime determined.

This is a preceeding of D107353.
The big picture is to let LoopIdiom deal with runtime-determined sizes.

Diff Detail

Event Timeline

eopXD created this revision.Jun 19 2021, 7:26 AM
eopXD requested review of this revision.Jun 19 2021, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2021, 7:26 AM
Whitney added inline comments.Jun 20 2021, 3:35 PM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
953

Do you think is beneficial to make this a forwarder in the meantime? like

const SCEV *StoreSizeSCEV = SE->getSCEV(StoreSize);
mayLoopAccessLocation(Ptr, Access, L, BECount, StoreSizeSCEV, AA, IgnoredStores);
983

If you plan to fully replace the original version, can you copy all the comments as well.

1058

Initialize to nullptr.

eopXD updated this revision to Diff 354662.Jun 26 2021, 2:43 AM
eopXD marked 3 inline comments as done.

Address comments.

eopXD abandoned this revision.Aug 3 2021, 7:57 AM

Closing this revision because D107353 is addressing these changes and additional ones that indicates its usage - Let the LoopIdiomRecognizePass deal with runtime-determined memset size.

eopXD reclaimed this revision.Aug 4 2021, 2:07 AM
eopXD retitled this revision from [NFC] [LoopIdiom] Let processLoopStridedStore able to take SCEV as Store Size to [NFC] [LoopIdiom] Let processLoopStridedStore take SCEV as Store Size.
eopXD edited the summary of this revision. (Show Details)
eopXD added a reviewer: lebedev.ri.

Reopen this patch since a NFC can be done prior to the main change (D107353) on LoopIdiom.

eopXD retitled this revision from [NFC] [LoopIdiom] Let processLoopStridedStore take SCEV as Store Size to [NFC] [LoopIdiom] Let processLoopStridedStore take StoreSize as SCEV instead of unsigned.Aug 4 2021, 2:10 AM
eopXD updated this revision to Diff 364015.Aug 4 2021, 2:35 AM

Rebase patch to latest main HEAD.

eopXD updated this revision to Diff 364105.Aug 4 2021, 7:40 AM

Fix clang-format warnings.

eopXD updated this revision to Diff 364126.Aug 4 2021, 8:28 AM

Fix more clang-format warnings.

Whitney added inline comments.Aug 4 2021, 10:59 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
916

Is it better to have line 916-918 after 921-923?

In what case, !MemsetSizeSCEV? Do we early exit more often than before?

983

comments start with uppercase.

1121

Is this really NFC?

eopXD updated this revision to Diff 364210.Aug 4 2021, 12:05 PM
eopXD marked an inline comment as done.

Address comments.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
916

The variable is not used other than forwarding the store size of the memset as SCEV into processLoopStridedStore. So I deleted this variable. Also the if-statement will never be triggered and is misleading.

1121

Right here the code does the exact same thing as above except no conversion on StoreSize is needed. (since its now a SCEV).

Created another getNumBytes function that takes StoreSize as SCEV, which better shows that this is a NFC.

eopXD edited the summary of this revision. (Show Details)Aug 4 2021, 12:06 PM
Whitney accepted this revision.Aug 4 2021, 12:15 PM

Please make sure clang-format is good for the new lines you changed.
Otherwise, LGTM.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
940–943

probably need to fix clang-format here.

This revision is now accepted and ready to land.Aug 4 2021, 12:15 PM

This does seem like a NFC refactor.
Thanks.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
985

I think this should just be inlined into users.

990–996
1000–1005

Same, i think this should just be inlined into users.

eopXD retitled this revision from [NFC] [LoopIdiom] Let processLoopStridedStore take StoreSize as SCEV instead of unsigned to [NFCI] [LoopIdiom] Let processLoopStridedStore take StoreSize as SCEV instead of unsigned.Aug 4 2021, 12:34 PM
eopXD updated this revision to Diff 364226.Aug 4 2021, 12:55 PM
eopXD marked 6 inline comments as done.

Address comments.

eopXD updated this revision to Diff 364227.Aug 4 2021, 1:02 PM

Delete redundent brackets added.

eopXD added a comment.Aug 4 2021, 1:05 PM

@lebedev.ri
If no further modification is required, please also accept this patch so that I can land it and move on to D107353.

lebedev.ri accepted this revision.Aug 4 2021, 1:13 PM

This looks fine i guess.