Page MenuHomePhabricator

PR47468: Fix findPHICopyInsertPoint, so that copies aren't incorrectly inserted after an INLINEASM_BR.
ClosedPublic

Authored by jyknight on Sep 17 2020, 3:42 PM.

Details

Summary

findPHICopyInsertPoint special cases placement in a block with a
callbr or invoke in it. In that case, we must ensure that the copy is
placed before the INLINEASM_BR or call instruction, if the register is
defined prior to that instruction, because it may jump out of the
block.

Previously, the code placed it immediately after the last def _or
use_. This is wrong, if the use is the instruction which may jump. We
could correctly place it immediately after the last def (ignoring
uses), but that is non-optimal for register pressure.

Instead, place the copy after the last def, or before the
call/inlineasm_br, whichever is later.

Diff Detail

Event Timeline

jyknight created this revision.Sep 17 2020, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 3:42 PM
jyknight published this revision for review.Sep 17 2020, 3:43 PM
jyknight added subscribers: manojgupta, jcai19, hans.
llvm/lib/CodeGen/PHIEliminationUtils.cpp
43

drop these extra parens?

45–58

Hoist above L34 and then reuse EHPadSuccessor?

52

if (DefsInMBB.contains(&*I)) { (I know it's the same impl...and the previous version used that method)

efriedma added inline comments.
llvm/lib/CodeGen/PHIEliminationUtils.cpp
57

Is it possible to have multiple INLINEASM_BR instructions in the same block? If it is, we need to find the first one, not the last one.

void added inline comments.Sep 17 2020, 4:35 PM
llvm/lib/CodeGen/PHIEliminationUtils.cpp
57

It shouldn't come to that. The callbr is a terminator, and an MBB with an INLINEASM_BR will have multiple successors, so there shouldn't be block merging.

jyknight marked 5 inline comments as done.Sep 18 2020, 9:33 AM
jyknight added inline comments.
llvm/lib/CodeGen/PHIEliminationUtils.cpp
57

Yes, both here and in SplitKit.cpp, we assume that there can only be one. (Note that this is not a _new_ assumption, it was also long the case that we assume only one invoke may be in a MBB,)

I'm not 100% happy with that, but it's definitely be the case for all MIR lowered from IR, and the blocks won't be merged so that property will be preserved.

jyknight updated this revision to Diff 292829.Sep 18 2020, 9:42 AM

Comment nits.

nickdesaulniers accepted this revision.Sep 18 2020, 10:18 AM
This revision is now accepted and ready to land.Sep 18 2020, 10:18 AM
This revision was landed with ongoing or failed builds.Sep 18 2020, 11:14 AM
This revision was automatically updated to reflect the committed changes.