This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Come back patch for the 'Assign register class for cross block values according to the divergence.'
ClosedPublic

Authored by alex-t on Oct 8 2019, 5:16 AM.

Details

Reviewers
rampitec
Summary

After https://reviews.llvm.org/D59990 submit several issues were discovered.
Changes in common code were preserved but AMDGPU specific part was reverted to keep the backend working correctly.

Discovered issues were addressed in the following commits:

This change brings back AMDGPU specific changes.

Diff Detail

Event Timeline

alex-t created this revision.Oct 8 2019, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 5:16 AM
rampitec added inline comments.Oct 8 2019, 10:00 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
10903

Why SetVector and not SmallPtrSet as usual?

10904

You can do if (!Visited.insert(V)) return false;

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

What's wrong with isAGPR() call?

arsenm added inline comments.Oct 8 2019, 10:33 AM
llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
659–660

Can you split this into a function?

659–660

This isn't a bool, so shouldn't start with has

669

.isPhysical()

685–698

This seems to be reproducing SIInstrInfo::getOpRegClass

707

SrcReg.isVirtual()

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

.isVirtual()

alex-t updated this revision to Diff 224099.Oct 9 2019, 10:39 AM

Changed according reviewers requests.

alex-t marked 9 inline comments as done.Oct 9 2019, 10:41 AM
This revision is now accepted and ready to land.Oct 9 2019, 11:18 AM

Thank you for persisting through this.

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp
746–747

LLVM coding style is to have the else on the same line as the }.

760–761

Same coding style remark.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
10903

The logic in this function seems needlessly convoluted. Instead of tracking Result, it should be possible to just return true in a number of places, right?

alex-t closed this revision.Oct 24 2019, 8:35 AM