This is an archive of the discontinued LLVM Phabricator instance.

[ScalarizeMaskedMemIntrin] Add new PM support
ClosedPublic

Authored by anna on Dec 6 2020, 7:55 PM.

Details

Summary

This patch adds new PM support for the pass (can be now used during
middle-end transforms). The old pass is remamed to
ScalarizeMaskedMemIntrinLegacyPass.

Tests-Run: code gen tests.

Diff Detail

Event Timeline

anna created this revision.Dec 6 2020, 7:55 PM
anna requested review of this revision.Dec 6 2020, 7:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2020, 7:55 PM
anna updated this revision to Diff 309805.Dec 6 2020, 7:56 PM

missed adding new header file to diff.

skatkov added inline comments.Dec 6 2020, 8:15 PM
llvm/include/llvm/LinkAllPasses.h
227

space. please be uniform with the current formatting.

Please consider separate renaming to Legacy to NFC patch.

llvm/lib/Transforms/Scalar/ScalarizeMaskedMemIntrin.cpp
849

You can get DL inside runImpl as it works identical in both legacy and new pass manager and does not depend on analysis.

anna added a comment.Dec 7 2020, 12:24 PM

Please consider separate renaming to Legacy to NFC patch.

I thought about it and saw that the renaming was done as part of same patch for new PM support for other passes as well (and I think it was intentional, in showing the need for the name update). :) If you feel strongly about it, I can do so.

anna added inline comments.Dec 7 2020, 5:27 PM
llvm/lib/Transforms/Scalar/ScalarizeMaskedMemIntrin.cpp
849

yeah, good point. will do.

skatkov accepted this revision.Dec 7 2020, 7:26 PM

Please consider separate renaming to Legacy to NFC patch.

I thought about it and saw that the renaming was done as part of same patch for new PM support for other passes as well (and I think it was intentional, in showing the need for the name update). :) If you feel strongly about it, I can do so.

I do not insist on it.

This revision is now accepted and ready to land.Dec 7 2020, 7:26 PM
aeubanks requested changes to this revision.Dec 7 2020, 8:39 PM
aeubanks added a subscriber: aeubanks.

can you remove this from the list in https://github.com/llvm/llvm-project/blob/6e614b0c7ed3a9a66428f342bf2a4b3700525395/llvm/tools/opt/opt.cpp#L463 and make sure that no check-llvm tests regress when -enable-new-pm is turned on by default? (I should rename that function)

This revision now requires changes to proceed.Dec 7 2020, 8:39 PM

Oh and add a -passes=scalarize-masked-mem-intrin RUN line to one of the tests just to make sure it works

Oh and add a -passes=scalarize-masked-mem-intrin RUN line to one of the tests just to make sure it works

agreed. makes sense.

anna added a comment.Dec 8 2020, 12:40 PM

can you remove this from the list in https://github.com/llvm/llvm-project/blob/6e614b0c7ed3a9a66428f342bf2a4b3700525395/llvm/tools/opt/opt.cpp#L463 and make sure that no check-llvm tests regress when -enable-new-pm is turned on by default? (I should rename that function)

Yes, pls rename the function since it actually represents which passes are not yet ported to NewPM.
Also, I have run the tests with enable-new-pm=true. However, there are 36 failures which are already failing with new-PM without my change (had to build both with and without my change to confirm this). Is there some buildbot that tracks the NewPM option? It will make it easier to catch if any testcases regress...

anna updated this revision to Diff 310316.Dec 8 2020, 12:41 PM

addressed review comments

can you remove this from the list in https://github.com/llvm/llvm-project/blob/6e614b0c7ed3a9a66428f342bf2a4b3700525395/llvm/tools/opt/opt.cpp#L463 and make sure that no check-llvm tests regress when -enable-new-pm is turned on by default? (I should rename that function)

Yes, pls rename the function since it actually represents which passes are not yet ported to NewPM.

https://reviews.llvm.org/D92818

Also, I have run the tests with enable-new-pm=true. However, there are 36 failures which are already failing with new-PM without my change (had to build both with and without my change to confirm this). Is there some buildbot that tracks the NewPM option? It will make it easier to catch if any testcases regress...

I'm working on reducing it to 0 so that we can have a bot that checks if it regresses.

aeubanks accepted this revision.Dec 8 2020, 12:43 PM
This revision is now accepted and ready to land.Dec 8 2020, 12:43 PM
anna updated this revision to Diff 310320.Dec 8 2020, 12:47 PM

missed one review comment: move DL initialization to runImpl.

anna added a comment.Dec 8 2020, 12:49 PM

Also, I have run the tests with enable-new-pm=true. However, there are 36 failures which are already failing with new-PM without my change (had to build both with and without my change to confirm this). Is there some buildbot that tracks the NewPM option? It will make it easier to catch if any testcases regress...

I'm working on reducing it to 0 so that we can have a bot that checks if it regresses.

Great, thanks!

This revision was landed with ongoing or failed builds.Dec 8 2020, 2:15 PM
This revision was automatically updated to reflect the committed changes.