This is an archive of the discontinued LLVM Phabricator instance.

Target hooks for custom COPY insertion.
ClosedPublic

Authored by alex-t on Sep 3 2019, 7:58 AM.

Details

Reviewers
rampitec
vpykhtin
Summary

PHI Elimination pass relies on TargetInstrInfo::isBlockPrologue/isTerminator target hooks to determine the COPY insertion points.
In some cases this is not sufficient. Divergent control flow requires instructions that belong to block prologue and consume registers defined by the PHI node in the same block.
2 new hooks were added to allow target to override the default COPY placement if necessary or fall through to the base class method otherwise.

Diff Detail

Event Timeline

alex-t created this revision.Sep 3 2019, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2019, 7:58 AM
vpykhtin added inline comments.Sep 3 2019, 10:19 AM
llvm/lib/CodeGen/PHIElimination.cpp
280

return inserted instruction and assign iterator to it to AfterPHIsIt

410

returned InsertPos and NewSrcInstr is the same, use only returned value, remove reference arg from the interface.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6398

use the same name for LastPHIIt arg as in the declaration

6400

auto

6407

search first user from the beginning of the block?

llvm/lib/Target/AMDGPU/SIInstrInfo.h
957

void -> MachineInstr*, remove reference to InsPt

963

remove reference from InsPt

Would be good to have test coverage either in the same patch or have dependent patch explicitly marked as such.

arsenm added inline comments.Sep 3 2019, 10:41 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1647

Should use Register instead of unsigned

Would be good to have test coverage either in the same patch or have dependent patch explicitly marked as such.

The patch really changes AMDGPU PHI elimination and leaves other targets behavior unaffected.
So the test that I will add make sense for AMDGPU only.
Is it okay?

Would be good to have test coverage either in the same patch or have dependent patch explicitly marked as such.

The patch really changes AMDGPU PHI elimination and leaves other targets behavior unaffected.
So the test that I will add make sense for AMDGPU only.
Is it okay?

Absolutely.

alex-t updated this revision to Diff 218911.Sep 5 2019, 6:59 AM

Changed according to the reviewers requests. Tests added.

alex-t marked 8 inline comments as done.Sep 5 2019, 7:00 AM
vpykhtin accepted this revision.Sep 5 2019, 7:04 AM

LGTM.

This revision is now accepted and ready to land.Sep 5 2019, 7:04 AM
MaskRay added a subscriber: MaskRay.Sep 5 2019, 7:22 AM
MaskRay added inline comments.
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1642

Delete to

1652

Delete to

alex-t closed this revision.Sep 10 2019, 5:05 AM
alex-t reopened this revision.Sep 13 2019, 10:35 AM
This revision is now accepted and ready to land.Sep 13 2019, 10:35 AM
alex-t updated this revision to Diff 220147.Sep 13 2019, 11:29 AM

Commit was reverted due to the EXPENSIVE_CHECKS failure.
Issue fixed

rampitec added inline comments.Sep 13 2019, 11:33 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6417

So you think it is always scalar and always 64 bit?

alex-t updated this revision to Diff 220166.Sep 13 2019, 1:27 PM

Empty block handling added

alex-t marked an inline comment as done.Sep 13 2019, 1:28 PM
alex-t added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6417

Since only case is SI_CF yes.

alex-t marked an inline comment as done.Sep 13 2019, 1:32 PM
alex-t added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6417

Oh... yes. in WAVE32 it will be S_MOV_B32_term

alex-t updated this revision to Diff 220167.Sep 13 2019, 1:42 PM

Wave32/Wave64 cases

rampitec added inline comments.Sep 13 2019, 1:53 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6422

impdef is just exec, you do not need to use exec_lo.

llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
204

S_MOV_B64?

rampitec added inline comments.Sep 13 2019, 2:01 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
6417

What ensures that is just SI_CF? Why it cannot be some another PHI?

alex-t updated this revision to Diff 220352.EditedSep 16 2019, 9:28 AM

Changed according the reviewer's request + some refactoring done.

alex-t marked 3 inline comments as done.Sep 16 2019, 9:29 AM
alex-t closed this revision.Sep 23 2019, 7:40 AM