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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:39-67
+/// Returns true and stores \p MI in \p Cst if it represents constant.
+bool isConst(MIPtr MI, MachineRegisterInfo &MRI, MIPtr &Cst) {
+  unsigned Opc = MI->getOpcode();
+  if (Opc == AMDGPU::G_CONSTANT) {
+    Cst = MI;
+    return true;
+  }
----------------
This feels like reinventing MIPatternMatch


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:83
+  }
+  if (MRI.hasOneNonDBGUse(Op2) && Op2Def->getOpcode() == Opc &&
+      isConst(Op1Def, MRI, Cst)) {
----------------
Opcode checks first?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:94
+  default:
+    return {0, 0, 0};
+  case AMDGPU::G_SMAX:
----------------
Technically 0 is a valid instruction opcode (PHI) but we ignore that most places


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:107
+  // 4 operand commutes of: min(max(Val, K0), K1)
+  if (MI.getOpcode() == MMMOpc.Min)
+    // Find K1 from outer instruction: min(max(...), K1) or min(K1, max(...))
----------------
Braces


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:151-152
+      OpcodeTriple.Med == AMDGPU::G_AMDGPU_UMED3) {
+    const APInt &KO_Imm = K0->getOperand(1).getCImm()->getValue();
+    const APInt &K1_Imm = K1->getOperand(1).getCImm()->getValue();
+    if (OpcodeTriple.Med == AMDGPU::G_AMDGPU_SMED3 && KO_Imm.sge(K1_Imm))
----------------
This will fail if the constant is a bitcasted G_FCONSTANT when the other patch extends isConst


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:166
+                              MinMaxToMed3MatchInfo &MatchInfo) {
+  MachineIRBuilder B(MI);
+  B.buildInstr(MatchInfo.Opc, {MI.getOperand(0)},
----------------
Should not construct new MachineIRBuilder


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

https://reviews.llvm.org/D90050