and, also update the function indirectCopyToAGPR() to ensure that it is called only on GFX908 sub-target.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Can you simplify the patch? I think all it is doing is (a) adding a new test and (b) simplifying the "if" on line 609 because the condition is always true. All the other changes make it harder to see what the patch is really doing.
For sure, I need to change review message now based on my latest update where I lately realized that "hasGFX90AInsts() is true for both GFX90A and GFX940 sub-targets".
However, the whole intention of this patch is to make sure that the function indirectCopyToAGPR() is meaningful only in case of GFX908, and should be called only in case of GFX908(). Hence updated indirectCopyToAGPR() accordingly. I am not sure what else can be simplified here apart from meaningfully updating the review message.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
554–560 | Either do this or don't. Adding another FIXME is just more clutter. Anyway I think it is more of a TODO than a FIXME since it is not really broken. | |
606 | I don't think you need a FIXME comment for this. It looks like the code is using the scavenger to try to find a low-numbered vgpr, and only using the reserved one if that fails. |
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
628 | Shouldn't this be Tmp = MIN (Tmp, Tmp2) if Tmp2 is a valid RegNo? |
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
628 | No. It should rotate 3 different VGPRs with each call. |
Either do this or don't. Adding another FIXME is just more clutter. Anyway I think it is more of a TODO than a FIXME since it is not really broken.