[PATCH] D90050: AMDGPU/GlobalISel: Add integer med3 combines

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks pretty good to me, just some questions inline.

Will we end up selecting v_med3 instructions even for uniform inputs? That would be bad.



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h:177
+                   MachineInstr *Inst) {
+    MI = const_cast<MachineInstr *>(Inst);
+    if (MI)
----------------
Why is the cast needed?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h:280
+template <typename LHS_P, typename RHS_P, bool Commutable = false>
+struct BinaryOpWithOpcode_match {
+  unsigned Opcode;
----------------
Might be simpler to make this a subclass of AnyBinaryOp_match?


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:349
+  MachineInstr *MI = MRI.getVRegDef(VReg);
+  if (TargetOpcode::G_CONSTANT != MI->getOpcode())
+    return nullptr;
----------------
Swap the operands: `MI->getOpcode() != TargetOpcode::G_CONSTANT`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:267
 
+Optional<AMDGPUPostLegalizerCombinerHelper::MinMaxMedOpc>
+AMDGPUPostLegalizerCombinerHelper::getMinMaxPair(unsigned Opc) {
----------------
I don't understand why you need to use Optional here. The default case could just be marked as unreachable.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:281
+
+template <class m_Cst>
+bool AMDGPUPostLegalizerCombinerHelper::matchMed(MachineInstr &MI,
----------------
Is this a template so that you can use it for floating point values in the future? Then shouldn't "CstRegMatch" be called "ICstRegMatch"?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:310-313
+  assert(OpcodeTriple && "Opcode not supported");
+  assert((OpcodeTriple->Med == AMDGPU::G_AMDGPU_SMED3 ||
+          OpcodeTriple->Med == AMDGPU::G_AMDGPU_UMED3) &&
+         "Opcode not supported");
----------------
I would remove these asserts.


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

https://reviews.llvm.org/D90050