[PATCH] D43433: [MIPS][MSA] Convert vector integer min/max opcodes to generic implementation

[Simon Dardis via Phabricator via llvm-commits]

sdardis accepted this revision.
sdardis added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks for the clean-up.

I've encountered a similar issue this week when following up an internal bug report/todo.

Some code archaeology shows that portions of the MIPS MSA support in LLVM use MipsISD opcodes which at the time did not have a corresponding target independent ISD opcode. Our target specific opcodes were added before the generic ISD opcodes were expanded to include SMAX & co.

The min_a & co. instructions are more specific to match, which is beyond the scope of this change, so they can be ignored.

Thanks again.
Simon



================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:897
 
-  if (Ty.is128BitVector() && Ty.isInteger()) {
-    // Try the following combines:
-    //   (vselect (setcc $a, $b, SETLT), $b, $a)) -> (vsmax $a, $b)
-    //   (vselect (setcc $a, $b, SETLE), $b, $a)) -> (vsmax $a, $b)
-    //   (vselect (setcc $a, $b, SETLT), $a, $b)) -> (vsmin $a, $b)
-    //   (vselect (setcc $a, $b, SETLE), $a, $b)) -> (vsmin $a, $b)
-    //   (vselect (setcc $a, $b, SETULT), $b, $a)) -> (vumax $a, $b)
-    //   (vselect (setcc $a, $b, SETULE), $b, $a)) -> (vumax $a, $b)
-    //   (vselect (setcc $a, $b, SETULT), $a, $b)) -> (vumin $a, $b)
-    //   (vselect (setcc $a, $b, SETULE), $a, $b)) -> (vumin $a, $b)
-    // SETGT/SETGE/SETUGT/SETUGE variants of these will show up initially but
-    // will be expanded to equivalent SETLT/SETLE/SETULT/SETULE versions by the
-    // legalizer.
-    SDValue Op0 = N->getOperand(0);
-
-    if (Op0->getOpcode() != ISD::SETCC)
-      return SDValue();
-
-    ISD::CondCode CondCode = cast<CondCodeSDNode>(Op0->getOperand(2))->get();
-    bool Signed;
-
-    if (CondCode == ISD::SETLT  || CondCode == ISD::SETLE)
-      Signed = true;
-    else if (CondCode == ISD::SETULT || CondCode == ISD::SETULE)
-      Signed = false;
-    else
-      return SDValue();
-
-    SDValue Op1 = N->getOperand(1);
-    SDValue Op2 = N->getOperand(2);
-    SDValue Op0Op0 = Op0->getOperand(0);
-    SDValue Op0Op1 = Op0->getOperand(1);
-
-    if (Op1 == Op0Op0 && Op2 == Op0Op1)
-      return DAG.getNode(Signed ? MipsISD::VSMIN : MipsISD::VUMIN, SDLoc(N),
-                         Ty, Op1, Op2);
-    else if (Op1 == Op0Op1 && Op2 == Op0Op0)
-      return DAG.getNode(Signed ? MipsISD::VSMAX : MipsISD::VUMAX, SDLoc(N),
-                         Ty, Op1, Op2);
-  } else if ((Ty == MVT::v2i16) || (Ty == MVT::v4i8)) {
+  if ((Ty == MVT::v2i16) || (Ty == MVT::v4i8)) {
     SDValue SetCC = N->getOperand(0);
----------------
Nit: C++ operator precedence, == is higher than ||, so the brackets around the type comparison are strictly redundant, though they do help make the checks obvious. That's a tiny cleanup for the original code.


Repository:
  rL LLVM

https://reviews.llvm.org/D43433