[PATCH] D134354: [AMDGPU][GlobalISel] Support mad/fma_mix selection

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGISel.td:156
 
+def gi_vop3_mad_mix_mods :
+    GIComplexOperandMatcher<s64, "selectVOP3PMadMixMods">,
----------------
Is this required? I am confused about whether you are now matching VOP3PMadMixMods in TableGen patterns, or only doing it from the C++ code in selectG_FMA.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:525
 
+bool AMDGPUInstructionSelector::selectG_FMA(MachineInstr &I) const {
+  assert(I.getOpcode() == AMDGPU::G_FMA || I.getOpcode() == AMDGPU::G_FMAD);
----------------
Call it selectG_FMA_FMAD.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:534
+      (IsFMA ? !Subtarget->hasFmaMixInsts() : !Subtarget->hasMadMixInsts())) {
+    return selectImpl(I, *CoverageInfo);
+  }
----------------
The usual convention seems to be to return false (with no braces) here, and have the caller do:
```
  if (selectG_FMA(I))
    return true;
  return selectImpl(I, *CoverageInfo);
```
Would that work? Or is there something special about the return false on line 575?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:558
+  if (!MatchedSrc0 && !MatchedSrc1 && !MatchedSrc2) {
+    return selectImpl(I, *CoverageInfo);
+  }
----------------
Return false? No braces.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:574
+
+  if (!constrainSelectedInstRegOperands(*MixInst, TII, TRI, RBI)) {
+    return false;
----------------
No braces.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:749
+                   .addReg(Src0);
+    if (!constrainSelectedInstRegOperands(*MIB, TII, TRI, RBI))
+      return false;
----------------
Is this related to the current patch? Could it be split out?


================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:2752
+  (v2f16 (DivergentBinFrag<build_vector> (f16 undef), (f16 VS_32:$src1))),
   (v2f16 (V_LSHLREV_B32_e64 (i32 16), SReg_32:$src1))
 >;
----------------
Pierre-vh wrote:
> foad wrote:
> > This looks weird. Why would we every want to generate V_LSHLREV_B32 with an sgpr operand?
> I just changed the pattern matching to also allow VGPR operands (which, IIRC, will be copied to the right regbank in any case). Not sure why the pattern uses a SGPR output though.
> 
> I did this change because it helped codegen in one test go from:
> ```
> v_mad_mix_f32 v0, v0, v1, v2 op_sel_hi:[1,1,1] clamp 
> v_cvt_f16_f32_e32 v0, v0 
> v_and_b32_e32 v1, 0xffff, v0 
> v_lshl_or_b32 v0, v0, 16, v1 
> ```
> to
> ```
> v_mad_mix_f32 v0, v0, v1, v2 op_sel_hi:[1,1,1] clamp
> v_cvt_f16_f32_sdwa v0, v0 dst_sel:WORD_1 dst_unused:UNUSED_PAD src0_sel:DWORD
> ```
> 
> Perhaps it'd be more adequate to duplicate the pattern and have another one that uses VReg input/output?
Maybe split this into a separate patch and we can discuss it there? It doesn't seem to be directly related to selecting the mad_mix instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134354