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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:534-535
+      (!Subtarget->hasMadMixInsts() && !Subtarget->hasFmaMixInsts()) ||
+      ((IsFMA && Subtarget->hasMadMixInsts()) ||
+       (!IsFMA && Subtarget->hasFmaMixInsts()))) {
+    return selectImpl(I, *CoverageInfo);
----------------
foad wrote:
> Personally I prefer to write this as `(IsFMA ? Subtarget->hasMadMixInsts() : Subtarget->hasFmaMixInsts())`.
> 
> Also I don't understand what the `(!Subtarget->hasMadMixInsts() && !Subtarget->hasFmaMixInsts())` part is for. Why can't the whole thing just be: `(IsFMA ? Subtarget->!hasFmaMixInsts() : !Subtarget->hasMadMixInsts())`?
That condition was imported from selectFMAD_FMA in the DAGISel and yeah, after looking at it more carefully it's indeed redundant, I rewrote it.


================
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))
 >;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134354