Page MenuHomePhabricator

AMDGPU: Move isSDNodeSourceOfDivergence() implementation to SITargetLowering
ClosedPublic

Authored by tstellar on Apr 30 2018, 8:57 PM.

Details

Summary

The code that handles ISD:Register and ISD::CopyFromReg assumes
the target is amdgcn, so this is broken on r600. We don't
need this analysis on r600 anyway so we can safely move
it to SITargetLowering.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Apr 30 2018, 8:57 PM
msearles added inline comments.
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
811 ↗(On Diff #144669)

Is the "// workitem.id.x workitem.id.y workitem.id.z" comment still relevant?

alex-t added a comment.May 3 2018, 4:49 AM

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

I would expect the TRI.isVGPR() call to crash if this was called from an R600 code path, so that made me think this was dead code.

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

I would expect the TRI.isVGPR() call to crash if this was called from an R600 code path, so that made me think this was dead code.

I agree isVGPR should be illegal to call for R600

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

Disable this for r600. It doesn't have the SGPR/VGPR split issue we want this for, so it doesn't really matter

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

Disable this for r600. It doesn't have the SGPR/VGPR split issue we want this for, so it doesn't really matter

My patch does this, but just for ISD:Register and ISD::CopyFromRegister, should I just disable this analysis completely for r600?

alex-t added a comment.Jun 4 2018, 3:34 AM
should I just disable this analysis completely for r600?

I'd like this better. It looks like cleaner way.
And yes it's okay to remove r600 registers from isSDNodeSourceOfDivergence

tstellar updated this revision to Diff 149790.Jun 4 2018, 9:51 AM

Completely disable isSDNodeSourceOfDivergence() for r600.

arsenm added inline comments.Jun 11 2018, 12:34 PM
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
785–790 ↗(On Diff #149790)

Just move this into SIISlLowering instead?

tstellar updated this revision to Diff 150890.Jun 11 2018, 9:42 PM

Move isSDNodeSourceOfDivergence() implementation from AMDGPUTargetLowering
to SITargetLowering.

tstellar retitled this revision from AMDGPU: Remove deadcode in isSDNodeSourceOfDivergence() to AMDGPU: Move isSDNodeSourceOfDivergence() implementation to SITargetLowering.Jun 11 2018, 9:44 PM
tstellar edited the summary of this revision. (Show Details)
arsenm accepted this revision.Jun 11 2018, 11:07 PM

LGTM

This revision is now accepted and ready to land.Jun 11 2018, 11:07 PM
This revision was automatically updated to reflect the committed changes.