This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Sink instructions with multiple users in a successor block.
ClosedPublic

Authored by wwei on Mar 14 2022, 4:05 AM.

Details

Summary

This patch tries to sink instructions when they are only used in a successor block.

This is a further enhancement patch based on Anna's commit:
D109700, which allows sinking an instruction having multiple uses in a single user.

In this patch, sink instructions with multiple users in a single successor block will be supported.

Diff Detail

Event Timeline

wwei created this revision.Mar 14 2022, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 4:05 AM
wwei requested review of this revision.Mar 14 2022, 4:05 AM

Generally, I'm fine with the direction here. My sole concern is potential compile time of scanning the entire user list. (i.e. say we have 100 thousand uses in one block, and the very last one is in another block) I could see us capping the number of scanned users (we do that a bunch of places) or would want to see some time time numbers showing we don't need to.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
4031–4032

I don't think you need this check at all. You can simple check that UniqueParent is non-null after the loop as that implies there must be at least one use.

nikic added a comment.Mar 14 2022, 1:12 PM

Generally, I'm fine with the direction here. My sole concern is potential compile time of scanning the entire user list. (i.e. say we have 100 thousand uses in one block, and the very last one is in another block) I could see us capping the number of scanned users (we do that a bunch of places) or would want to see some time time numbers showing we don't need to.

Agreed, this needs a cutoff.

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

The mixed usage of UserParent and UniqueParent here is odd. You'll want to move UserParent inside the loop and only use one variable outside it.

4062–4063

It might make sense to move these checks to the first time we populate the user block, so we e.g. don't waste time visiting all users of an instruction in the same block.

nikic requested changes to this revision.Mar 15 2022, 1:19 AM
This revision now requires changes to proceed.Mar 15 2022, 1:19 AM
wwei updated this revision to Diff 415427.Mar 15 2022, 7:55 AM
wwei added inline comments.Mar 15 2022, 7:59 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
4031–4032

check removed

4035

UniqueParent removed

4062–4063

Thanks, these checks moved into for loop

wwei added a comment.Mar 15 2022, 8:06 AM

Generally, I'm fine with the direction here. My sole concern is potential compile time of scanning the entire user list. (i.e. say we have 100 thousand uses in one block, and the very last one is in another block) I could see us capping the number of scanned users (we do that a bunch of places) or would want to see some time time numbers showing we don't need to.

Agreed, this needs a cutoff.

I have added an option to limit the number of scanned users, the potential compile time issue can be fixed.

reames requested changes to this revision.Mar 15 2022, 8:50 AM

Fix the indentation so that the code is readable + inline comments.

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

This should default to more like 30 to be in sync with other such cutoffs.

4063

The flow of this bit makes no sense to me, but that might be because I'm misreading due to the indentation issues.

This revision now requires changes to proceed.Mar 15 2022, 8:50 AM
wwei updated this revision to Diff 415483.Mar 15 2022, 9:58 AM

Fix the indentation

wwei added inline comments.Mar 15 2022, 10:02 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
142

modify the default value to 32

4063

In order to run these checks only once, don't need to check them every iterations. So I add if(NumUsers == 0) here.

wwei updated this revision to Diff 415862.Mar 16 2022, 9:21 AM

update some comments and make the changes in first-order-recurrence.ll clean

nikic accepted this revision.Mar 17 2022, 1:28 AM

LGTM

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

Precommit this test addition please.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 17 2022, 9:09 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.