Page MenuHomePhabricator

[AMDGPU] Insert waterfall loops for divergent calls
ClosedPublic

Authored by Flakebi on Sep 25 2020, 3:54 AM.

Details

Summary

Extend loadSRsrcFromVGPR to allow moving a range of instructions into
the loop. The call instruction is preceded by copies into physical
registers which should be part of the waterfall loop, as the registers
can be overwritten by the call.

Diff Detail

Event Timeline

Flakebi created this revision.Sep 25 2020, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 3:54 AM
Flakebi requested review of this revision.Sep 25 2020, 3:54 AM
madhur13490 added inline comments.Tue, Sep 29, 1:16 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
4811

Dump() is not required.

5037

Should this block be executed for AGPRs too? If this is meant only for VGPRs then !SGPR is not correct.

Flakebi updated this revision to Diff 294914.Tue, Sep 29, 2:42 AM

Remove debug dumps

Flakebi added inline comments.Tue, Sep 29, 2:48 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
4811

Thanks, I forgot to remove them.

5037

I don’t know how AGPRs work and documentation seems to be scarce, the check for calls is the same as for image operations above.
It seems like AGPRs can be copied to VGPRs but not to SGPRs (https://reviews.llvm.org/D68358#change-cnuV0NuhUhzL), so I think calling a function pointer that is stored in AGPRs should copy them to VGPRs and insert a waterfall loop.

madhur13490 added inline comments.Tue, Sep 29, 3:10 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
5037

My main concern is that this check is over relaxing and allows all non-SGPR classes. This also includes any future register class we may add. AGPR is just an example. What about enabling it just for VGPR for now and put a TODO?

nhaehnle added inline comments.Wed, Sep 30, 4:43 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
5037

Dest is the destination we're calling, i.e. the function pointer. The point of the logic, AFAIU, is that if the function pointer is non-uniform, we need to do something about that. AGPRs are non-uniform... so I'd say !isSGPR is correct.

madhur13490 accepted this revision.Sun, Oct 4, 8:31 AM
This revision is now accepted and ready to land.Sun, Oct 4, 8:31 AM
Flakebi updated this revision to Diff 296931.Thu, Oct 8, 5:15 AM

Fix return value handling, also need to copy COPYs following the call.

This revision was automatically updated to reflect the committed changes.