[PATCH] D115945: [AMDGPU][GlobalISel] Eliminate cross regbank copies of constants

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp:371
+
+  // Check if all uses of constant are copies to same regbank
+  for (MachineInstr &Use : MRI.use_nodbg_instructions(Dst)) {
----------------
foad wrote:
> arsenm wrote:
> > foad wrote:
> > > arsenm wrote:
> > > > This level of heuristic could have/should have been done by greedy regbankselect.
> > > > 
> > > > However, I think we want something a bit smarter here. We should rematerialize all uses that are inline constants. For literal constants, we can make a code size tradeoff based on the number of uses and if other operands are scalars
> > > > For literal constants, we can make a code size tradeoff based on the number of uses and if other operands are scalars
> > > 
> > > Can't we do something simpler here and leave that kind of tradeoff to SIFoldOperands, which already makes that kind of decision?
> > No, because these copies are also confusing selection patterns. I think we need to do both
> I agree we need to do *something* here, it's just not clear to me that any kind of heuristic is useful. The code size tradeoff is whether to fold literal constants into their uses or not, but here we're talking about whether to copy a constant from sgpr to vgpr or not. I don't really see how they relate.
> 
> Why don't we just remat *all* constants, by removing the hasOneNonDBGUse check from AMDGPURegisterBankInfo::canRematerialize? What's the worst that can happen -- you'll get a non-inlinable constant materialized in both an sgpr and a vgpr, and SIFoldOperands will be unable to replace uses of that vgpr with the sgpr because it does not track the fact that they hold the same constant value?
Yes. Plus I would like to write a new version of SIFoldOperands which collects all operands and considers the entire instruction to choose the optimal set, which could replace v constants with s constants


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115945/new/

https://reviews.llvm.org/D115945