[PATCH] D104874: [AMDGPU] Add S_MOV_B64_IMM_PSEUDO for wide constants

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Probably should rename the pass to be a bit more generic. There are other optimizations we could potentially do here (e.g. this is really where we probably should be optimizing m0 initializations instead of what we do now, and maybe some AGPR-VGPR fixups)



================
Comment at: llvm/lib/Target/AMDGPU/GCNCombineTupleInits.cpp:87
+  MachineInstr *Def1 = nullptr;
+  int64_t Init = 0;
+
----------------
uint64_t


================
Comment at: llvm/lib/Target/AMDGPU/GCNCombineTupleInits.cpp:107
+      Def1 = &I;
+      Init |= I.getOperand(1).getImm() << 32;
+      break;
----------------
Need to make sure this shift is done as uint64_t to avoid UB


================
Comment at: llvm/lib/Target/AMDGPU/GCNCombineTupleInits.cpp:115
+
+  LLVM_DEBUG(dbgs() << "Comining:\n  " << *Def0 << "  " << *Def1 << "    =>\n");
+
----------------
Typo Comining


================
Comment at: llvm/lib/Target/AMDGPU/GCNCombineTupleInits.cpp:133
+
+  LLVM_DEBUG(dbgs() << "  " << *NewI << '\n');
+
----------------
Don't need the newline


================
Comment at: llvm/lib/Target/AMDGPU/GCNCombineTupleInits.cpp:154-155
+      continue;
+    const TargetRegisterClass *RC;
+    RC = MRI->getRegClass(Reg);
+    if (RC->MC->getSizeInBits() != 64 || !TRI->isSGPRClass(RC))
----------------
Initialize and define at the same time?


================
Comment at: llvm/lib/Target/AMDGPU/GCNCombineTupleInits.cpp:156
+    RC = MRI->getRegClass(Reg);
+    if (RC->MC->getSizeInBits() != 64 || !TRI->isSGPRClass(RC))
+      continue;
----------------
Why not also handle the VALU cases?


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

https://reviews.llvm.org/D104874