Constants with a single use or G_IMPLICIT_DEF can be repaired by simply
cloning instruction that defined them with the appropriate register bank.
Details
Diff Detail
Unit Tests
Event Timeline
This is another attempt at trying to get of cross reg bank copies for which I needed manual checks when matching (like in https://reviews.llvm.org/D110937, https://reviews.llvm.org/D98040)
I'm wondering if this would be considered more of a hack then an appropriate way of getting rid of these annoying copies.
We get decreased instruction count in a lot of test.
I'll highlight below (inline) cases that I noticed where instruction count actually increases.
llvm/test/CodeGen/AMDGPU/GlobalISel/insertelement.i16.ll | ||
---|---|---|
2099 | This is because of SMEMtoVectorWriteHazards on gfx10. Inserted by GCNHazardRecognizer::fixSMEMtoVectorWriteHazards. | |
llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.intersect_ray.ll | ||
74 | Longer code due to a vgpr (v5) used for const. | |
llvm/test/CodeGen/AMDGPU/GlobalISel/lshr.ll | ||
31–34 | SIFoldOperands won't inline this constant because it has multiple uses. It does this because this might increase code size. Previously this were two different constants (two different instructions) but they had same value. SDag also does not inline const but uses sgpr. | |
llvm/test/CodeGen/AMDGPU/cttz.ll | ||
974–982 | Extra vgpr used here (now 3 was 2) |
Last time I thought about this I thought it would be easier to have the post-regbank combiner handle this. In some situations it makes most sense to completely rematerialize the constant value in each regbank, not just reassign it. If you have an inline constant, it would be better to just emit a new constant for each bank. For multiple uses of literals its trickier since there's a code size or instruction count tradeoff based on the uses
llvm/include/llvm/CodeGen/GlobalISel/RegBankSelect.h | ||
---|---|---|
324 ↗ | (On Diff #386883) | Typo prevously |
325 ↗ | (On Diff #386883) | I would expect to make the right decision upfront, not have to go back and erase copies |
llvm/include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h | ||
626–631 ↗ | (On Diff #386883) | I think this is way too specific of a target hook |
I've considered this regbank combiner solution before but it was too late for D98040. Thankfully those cases are single use constants needed for offset for load/stores, a case which can be easily solved in RegBankSelect.
Now updating of regbanks is split between RegBankSelect with no target hooks and AMDGPURegBankCombiner.
As discussed offline, maybe split this patch into two?
- Just the generic change to RegBankSelect
- Add the AMDGPU combine
Removed combiner. Now only constants that have single use are handled here.
Combiner that handles constants with multiple uses is now: https://reviews.llvm.org/D115945
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp | ||
---|---|---|
166 | the same applies for G_FCONSTANT and G_IMPLICIT_DEF, but I'm not sure this should be unconditionally done for all targets, and in all situations. For instance on AMDGPU we may want to choose a code size tradeoff with non-inlineable constants |
I'm currently thinking rematerialized in regbankselect is a good strategy. We would still want to have a post-isel operand optimizer pass try to re-scalarize constants when there's open constant bus use.
llvm/include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h | ||
---|---|---|
631 ↗ | (On Diff #405271) | It would be better to report the rematerializability of an opcode as part of the reported mapping rather than add another callback for it |
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp | ||
166–170 | I think it would be cleaner to handle this as a separate RepairingPlacement case in applyMapping |
the same applies for G_FCONSTANT and G_IMPLICIT_DEF, but I'm not sure this should be unconditionally done for all targets, and in all situations. For instance on AMDGPU we may want to choose a code size tradeoff with non-inlineable constants