This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Improve TryToSinkInstruction with multiple uses
ClosedPublic

Authored by anna on Sep 13 2021, 9:03 AM.

Details

Summary

This patch allows sinking an instruction which can have multiple uses in a
single user. We were previously over-restrictive by looking for exactly one use,
rather than one user.

Also added an API for retrieving a unique undroppable user.

Diff Detail

Event Timeline

anna created this revision.Sep 13 2021, 9:03 AM
anna requested review of this revision.Sep 13 2021, 9:03 AM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
anna updated this revision to Diff 372275.Sep 13 2021, 9:07 AM

removed stray change in file

Please split the rename into a preparatory change

anna added inline comments.Sep 13 2021, 9:09 AM
llvm/include/llvm/IR/Value.h
456–457

This comment wasn't true even before the patch. We are traversing the entire use list here to find the "non-droppable use". I can land this separately.

anna added a comment.Sep 13 2021, 9:10 AM

Please split the rename into a preparatory change

I don't follow. This is not a rename. It is an API change where we return a unique *User* instead of a use.

mkazantsev added inline comments.Sep 13 2021, 7:58 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3788

Can we at least allow it if they come from same block? It's typical for switches.

anna added inline comments.Sep 14 2021, 6:10 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3788

yes, will do.

anna updated this revision to Diff 372526.Sep 14 2021, 11:53 AM

addressed review comment. Added test

The change looks reasonable to me.

llvm/lib/IR/Value.cpp
169

Might as well iterate over users() and avoid the three U.getUser() calls?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3775–3776

Dropping dyn_ should be fine here -- user of instruction is always an instruction.

3784

nit: Missing space after =. Also, please add braces for the for, as the inner code has braces.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
165 ↗(On Diff #372526)

Is the cast here needed?

llvm/test/Transforms/InstCombine/sink_instruction.ll
82

Please precommit tests, including generation of all check lines in this file using update_test_checks.

anna updated this revision to Diff 372764.Sep 15 2021, 11:43 AM
anna marked 4 inline comments as done.

addressed review comments.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
165 ↗(On Diff #372526)

not needed.

Please split the rename into a preparatory change

I don't follow. This is not a rename. It is an API change where we return a unique *User* instead of a use.

Please disregard. My actual question was intended to be more along the lines of:
"do we actually want to completely replace that API like this",
but i'm not really seeing a reason not to.

nikic accepted this revision.Sep 15 2021, 12:14 PM

LGTM

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3788

The "more than one use" comment isn't quite accurate anymore.

This revision is now accepted and ready to land.Sep 15 2021, 12:14 PM
anna added a comment.Sep 15 2021, 12:55 PM

Please split the rename into a preparatory change

I don't follow. This is not a rename. It is an API change where we return a unique *User* instead of a use.

Please disregard. My actual question was intended to be more along the lines of:
"do we actually want to completely replace that API like this",
but i'm not really seeing a reason not to.

Ah. Yes, I went through the same process when deciding to replace the original API :)

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3788

Good point, will update comment before landing.

This revision was landed with ongoing or failed builds.Sep 15 2021, 1:40 PM
This revision was automatically updated to reflect the committed changes.
hctim added a subscriber: hctim.Sep 15 2021, 2:29 PM

Early warning signs, looks like it broke the sanitizer fast bot: https://lab.llvm.org/buildbot/#/builders/5/builds/11759

Unclear whether this patch is fully responsible, or the bot's ccache is messed up.

hctim added a comment.Sep 15 2021, 2:31 PM

Breakage on regular fastbot was probably occluded as the bot was already red, but it's broken there too: https://lab.llvm.org/buildbot/#/builders/109/builds/22459

anna added a comment.Sep 15 2021, 2:50 PM

Thank you for the failure notifications. I have landed the fixes in tree:
https://reviews.llvm.org/rG3273430406c186f1f2af597adeedf21d4fa52b18
https://reviews.llvm.org/rGb6cb03e6b94d91a1757d41d4b0f139166ae15de0

If it still fails, I will revert the change and investigate tomorrow.

So, getSingleUndroppableUser is needed for AssumeBundleBuilder. Added the API back and also continued using it in AssumeBundleBuilder (the second commit above).

Sorry for the noise here. EoD is never a good time to land. Should have waited till ninja check passed.

So i guess my question was correct then :/
I would suggest to reduce this change to an introduction of the new method (while keeping the old one!), and porting a single user (instcombine pass) to use it.

anna reopened this revision.Sep 16 2021, 7:13 AM
anna edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Sep 16 2021, 7:15 AM
anna updated this revision to Diff 372935.Sep 16 2021, 7:15 AM

Narrower change compared to previous one: getUniqueUndroppableUser API introduced is only used within InstCombine.

anna requested review of this revision.Sep 16 2021, 7:16 AM

The change is narrower now and updated test cases in SLPVectorizer (no surprises in the updates).

nikic accepted this revision.Sep 16 2021, 12:23 PM

Still LGTM, though I'd be interested in knowing what was wrong with the AssumeBundleBuilder case. It's not entirely obvious to me why the switch to getUniqueUndroppableUser() caused issues there.

This revision is now accepted and ready to land.Sep 16 2021, 12:23 PM
anna added a comment.Sep 21 2021, 7:02 AM

Still LGTM, though I'd be interested in knowing what was wrong with the AssumeBundleBuilder case. It's not entirely obvious to me why the switch to getUniqueUndroppableUser() caused issues there.

I will check that separately and address. The lit testcase may just require an update. However, the unit-test which fails building (because of absence of the use API: getSingleUndroppableUse)
calls AbstractCallSite(const Use *U) constructor. That still requires the getSingleUndroppableUse to be around.

This revision was landed with ongoing or failed builds.Sep 21 2021, 7:04 AM
This revision was automatically updated to reflect the committed changes.