This is an archive of the discontinued LLVM Phabricator instance.

[Instcombine]Transform reduction+(sext/zext(<n x i1>) to <n x im>) to [-]zext/trunc(ctpop(bitcast <n x i1> to in)) to im.
ClosedPublic

Authored by ABataev on Jul 7 2021, 1:16 PM.

Details

Summary

Some of the SPEC tests end up with reduction+(sext/zext(<n x i1>) to <n x im>) pattern, which can be transformed to [-]zext/trunc(ctpop(bitcast <n x i1> to in)) to im.

https://alive2.llvm.org/ce/z/TBZbdZ
https://alive2.llvm.org/ce/z/N6qJGz

Diff Detail

Event Timeline

ABataev created this revision.Jul 7 2021, 1:16 PM
ABataev requested review of this revision.Jul 7 2021, 1:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 1:16 PM

Yes, checked it via alive2 before uploading

Yes, checked it via alive2 before uploading

While not required, since you already had it, it is really good to make patch description more useful by adding such details there.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2006

Extension is optional: https://alive2.llvm.org/ce/z/N6qJGz
Please either use m_ZExtOrSExtOrSelf(), or if it's already handled elsewhere add a comment about that.

Yes, checked it via alive2 before uploading

While not required, since you already had it, it is really good to make patch description more useful by adding such details there.

Sure.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2006

Did not see such a pattern but will check if it is handled already and use add m_ZExtOrSExtOrSelf.

ABataev updated this revision to Diff 357218.Jul 8 2021, 7:15 AM

Address comments

lebedev.ri edited the summary of this revision. (Show Details)Jul 8 2021, 7:23 AM
lebedev.ri added inline comments.
llvm/test/Transforms/InstCombine/reduction-add-sext-zext-i1.ll
1

Please precommit

65

Could use a test where the zext/sext has extra use and doesn't go away.

lebedev.ri accepted this revision.Jul 8 2021, 7:24 AM

LG with nits

This revision is now accepted and ready to land.Jul 8 2021, 7:24 AM
ABataev added inline comments.Jul 8 2021, 7:25 AM
llvm/test/Transforms/InstCombine/reduction-add-sext-zext-i1.ll
1

Will do.

65

Will add

This revision was landed with ongoing or failed builds.Jul 8 2021, 7:57 AM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Jul 8 2021, 11:42 AM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2020

Seems to be a recurring pattern in these folds, but why does this call replaceInstUsesWith and eraseInstFromInstruction, rather than doing just return replaceInstUsesWith()?

ABataev added inline comments.Jul 8 2021, 11:45 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2020

I'm not quite familiar with these interfaces, just copied from other places. Do you suggest replacing it with just return replaceInstUsesWith(CI, Res);?

nikic added inline comments.Jul 8 2021, 12:52 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2020

Yeah, that should be sufficient.

ABataev added inline comments.Jul 8 2021, 12:53 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2020

Ok, thanks for the advice, will do