[AMDGPU] Reserve SGPR pair when long branches are present

[crobeck]

Summary

After applying recent register allocation patches the compiler generates a register scavenging error. Branch Relaxation runs after RA, but RA doesn't allocate enough registers to plan for the case where an indirect branch ends up being needed during branch relaxation. This causes a “did not find scavenging index” assert to be hit from assignRegToScavengingIndex from within RegScavenger.

In this patch we estimate before RA whether an indirect branch is likely to be needed, and reserve 2 SGPRs if the branch distance is found to be above a threshold. This is difficult as you often don't have an accurate idea of the code size and branch distance before register allocation and when you would need to reserve the registers. We therefore make the distance calculation a reduced complexity approximation and add a tuning factor on the threshold through the -amdgpu-long-branch-factor command line argument.

[crobeck]

This comment was removed by crobeck.

[crobeck]

fix a type mismatch

[crobeck]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1453–1455

Yes, you're right. I've made the checks more explicit.

[crobeck]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1453–1455

Would it be worth adding a test with amdgpu-num-sgpr to see what happens when 100% of SGPRs are used?

That's a good idea. At the very least good to understand what happens when were maxed out on SGPRs.

[crobeck]

add test for when maxed out on SGPRs

[crobeck]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1453–1455

This ended up being kind of a mess trying to do it with the amdgpu-num-sgpr attribute so just used inline asm to use up all the registers

[crobeck]

Any further comments on this? @arsenm Do you think we need to update the distance calculation or are OK to leave as is?

[arsenm]

llvm/lib/Target/AMDGPU/GCNPreRABranchDistance.cpp

36 ↗

(On Diff #520388)

I think you should go for fuzzier. Drop the block map and all that from BranchRelaxation. Just do something dead simple like number of instructions * 8 * fuzz ~= branch distance

[crobeck]

Reduce complexity of branch distance calculation substantially - we now just approximate branch size by assuming 8 bytes per instruction. Also updated pass name to be more representative of what the pass does.

[crobeck]

clean up unused header files and an unused function

[arsenm]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

46

uint64_ts?

78–79

Clear at exit instead?

86

Should skip debug and meta instructions, otherwise this violates the cardinal rule that debug info shouldn't change codegen

121

const

135–137

This should be moved into a TRI reserve registers helper like the others.

Also, would be a bit safer to invert this by reserving by default and having this pass *remove* the reserved register

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1429

The reserved registers should already be frozen before PEI. I think the current freezeReservedRegs call here was just never updated to use the new incremental reserveReg

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

2555

We should also fix this fixme at some point

llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-long-branch-reg.ll

67

Add a test to show no change for debug instructions

[arsenm]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

31

If you're going to use FP for this, might as well use double

[cdevadas]

Add a test for MIR serialization.

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

107

Capitalize.

118

Capitalize.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1445

Unify the freezeReservedRegs at the end of the function?

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

2553

const

[crobeck]

address review comments, add additional debug instruction test

[crobeck]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

135–137

We're not actually reserving the register here. We're just setting the flag that we need to reserve the register in TRI. But, I can move this if it makes more sense there.

If we instead invert it we then need to always run through everything and prove there isn't a long branch as opposed to exiting once if we find a long branch.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1429

Just set both calls to reserveReg then?

1445

Pending updating reserveReg call above.

[crobeck]

Update reserveReg calls in SIFrameLowering

[crobeck]

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp

1429

I think I've convinced myself these can both just be reserveReg calls.

[crobeck]

Swap out reserve register routine for TRI helper function

[crobeck]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

135–137

This should be cleaner now with a call into TRI instead of grabbing my own registers.

[arsenm]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

87

Just isMetaInstruction is fine, it's a superset of isDebugIstr

103

Don't need a specific instruction offset, just the block distances are good enough

[crobeck]

remove specific instruction offsets from distance calc

[crobeck]

variable, pointer/reference clean up

[crobeck]

invert to reserve register by default and then have pass remove the register if unneeded

[arsenm]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

117–118

I don't see how this can fail unless you're using amdgpu-num-sgprs (and this is one of the reasons it should be removed)

120

I would assume this is already noregister if this failed

I think you misunderstood what I was saying about reserve first and release later. I meant in the compile pipeline, not in the pass itself. You can find the reserved register in finalizeLowering, and then have this pass unset and un-reserve. I don't know if we have an exposed helper to clear reserved registers. You can do that as a follow up,

121

If you actually did modify the MFI register this should be a return true

144

This should be return true if you modified MFI

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

2623

Should also skip the enterBasicBlockEnd above

[crobeck]

remove unnecessary set functions

[crobeck]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

117–118

It failed in some test cases where all the SGPRs were already used from inline asm

120

Yes, you're right I can remove that set function.

Got it. OK, that makes sense.

[crobeck]

skipping some of register scavenging if we already have a reserved register

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

2623

There is some init of MBB in enterBasicBlockEnd that breaks things if I skip it entirely. But I think I can get away will skipping some of it.

[arsenm]

llvm/lib/Target/AMDGPU/GCNPreRALongBranchReg.cpp

117–118

oh right

[crobeck]

rG853b2a84cb9902725752e9603011041ebe33c7bf