[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/AMDGPUInstructionSelector.cpp:534-535
+      (!Subtarget->hasMadMixInsts() && !Subtarget->hasFmaMixInsts()) ||
+      ((IsFMA && Subtarget->hasMadMixInsts()) ||
+       (!IsFMA && Subtarget->hasFmaMixInsts()))) {
+    return selectImpl(I, *CoverageInfo);
----------------
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())`?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:753
+      return false;
+    }
+
----------------
Don't need the braces.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:759
+              .addReg(TmpReg);
+    if (!constrainSelectedInstRegOperands(*MIB, TII, TRI, RBI)) {
+      return false;
----------------
Don't need the braces.


================
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))
 >;
----------------
This looks weird. Why would we every want to generate V_LSHLREV_B32 with an sgpr operand?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134354