[PATCH] D124843: AMDGPU: Add G_AMDGPU_MAD_64_32 instructions

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1641
+
+  bool IsUnsigned = MI.getOpcode() == AMDGPU::G_AMDGPU_MAD_U64_U32;
+  LLT S1 = LLT::scalar(1);
----------------
arsenm wrote:
> Most places invert this and use IsSigned (saves a few characters I guess)
How strongly do you feel about that? I used `IsUnsigned` because it puts the more common opcode first in ternary ?: operators.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1649-1651
+    if (MachineInstr *Def = getDefIgnoringCopies(Src2, MRI)) {
+      if (Def->getOpcode() == TargetOpcode::G_CONSTANT &&
+          Def->getOperand(1).getCImm()->isZeroValue())
----------------
arsenm wrote:
> Can you use m_ZeroInt?
Good point.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1680-1692
+    if (!DstOnValu) {
+      const TargetRegisterClass *Constrained =
+          constrainGenericRegister(DstHi, AMDGPU::VGPR_32RegClass, MRI);
+      (void)Constrained;
+      assert(Constrained && "Failed to constrain readfirstlane src reg");
+
+      Register SMulHi = MRI.createVirtualRegister(&AMDGPU::SReg_32RegClass);
----------------
arsenm wrote:
> Can you use constrainOpWithReadfirstlane?
Not quite, because MulHi may actually be used in two separate places. It really feels more logical to insert the readfirstlane when the producing instruction is built. However, I looked into factoring out the code that creates readfirstlane (and added a new parent commit for that).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1773
+      Carry = B.buildConstant(CarryType, 0).getReg(0);
+      MRI.setRegBank(Carry, CarryBank);
+    }
----------------
arsenm wrote:
> You could use ApplyRegBankMapping to avoid all of these setRegBank calls
I looked into that, but ApplyRegBankMapping unconditionally applies the same bank to everything. This code needs different banks throughout, and it doesn't seem possible to adjust ApplyRegBankMapping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124843