[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/AMDGPUInstructionSelector.cpp:3168
+  case AMDGPU::G_AMDGPU_MAD_I64_I32:
+    return selectG_AMDGPU_MAD_64_32(I);
   case TargetOpcode::G_INTTOPTR:
----------------
arsenm wrote:
> Don't the selector patterns work? This one should work without much fuss
Which selector patterns do you mean? SelectionDAG has custom code for this as well because of the vcc_out (see AMDGPUDAGToDAGISel::SelectMAD_64_32).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1624
 
+bool AMDGPURegisterBankInfo::applyMappingMAD_64_32(
+    const OperandsMapper &OpdMapper) const {
----------------
arsenm wrote:
> I would assume we form this after regbank select and don't need to legalize it based on the regbank
No, see the follow-up patch in the stack which introduces custom legalization for G_MUL.

I did look into combining the results of the generic G_MUL legalization, but the patterns get really messy and expensive. It's cleaner (and faster in terms of compile-time) to generate the right code directly from legalization.

I suppose one could look at a hybrid of (custom G_MUL legalization into non-target-specific GMIR, but with better suitable patterns) + (matching GMIR into V_MAD_U64_U32 after RegBankSelect). However, merely matching G_MUL + G_UMULH + G_ADDO + G_ADDE is already quite expensive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124843