[PATCH] D150788: [AMDGPU][GlobalISel] Rematerialze constants with different regbank

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:525-526
+    MachineInstr *DefMI = MRI.getVRegDef(OpReg);
+    if ((DefMI->getOpcode() == TargetOpcode::G_CONSTANT ||
+         DefMI->getOpcode() == TargetOpcode::G_FCONSTANT) &&
+        !MO.isDef() && MRI.getType(OpReg).getSizeInBits() <= 32) {
----------------
Do we have some sort of isLeaf or isConstantLike helper? This should also at minimum cover G_FRAME_INDEX and G_GLOBAL_VALUE. I think it makes sense in some cases with inputs too


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:250-261
+  const unsigned OpIdx = RepairPt.getOpIdx();
+  OpdMapper.createVRegs(OpIdx);
+
+  // Clone the instruction but update the regbank.
+  MachineInstr *NewMI = OldMI->getMF()->CloneMachineInstr(OldMI);
+  NewMI->getOperand(0).setReg(*OpdMapper.getVRegs(OpIdx).begin());
+  LLVM_DEBUG(dbgs() << "Clone with new regbank: " << *OldMI << '\n');
----------------
mbrkusanin wrote:
> Regbank's MIRBuilder does not have CSEInfo so this will crate new G_(F)CONSTANT for each use. I tried an idea to avoid that where InsertPoint for rematerializations would be changed to always be after old G_(F)CONSTANT, so we would only need to check instruction after the current/old one if it is a copy with the appropriate regbank (created earlier). This did make .mir test look shorter in some cases but it did not have any real improvements for final ISA, only shuffled instructions.
This is just a bug. I've been meaning to thread a MIRBuilder through all these APIs. MIRBuilders should be constructed once at pass entry to preserve CSEInfo


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:527
+         DefMI->getOpcode() == TargetOpcode::G_FCONSTANT) &&
+        !MO.isDef() && MRI.getType(OpReg).getSizeInBits() <= 32) {
+      // Constants can be rematerialzed.
----------------
mbrkusanin wrote:
> Maybe this should be a target hook? Lit tests from other targets are not affected by this change, but check for size<32 is because of lack of v_mov_b64.
I don't follow the v_mov_b64 part, the size shouldn't matter. We can rematerialize as 2 pieces easily (also, we do technically have the infrequently used V_MOV_B64_PSEUDO). In any case that's beyond the concern of this action


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

https://reviews.llvm.org/D150788