[PATCH] D87836: [ARM] Fold select_cc(vecreduce_[u|s][min|max], x) into VMINV or VMAXV

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5208
+  EVT VectorScalarType =
+      FalseVal->getOperand(0)->getValueType(0).getVectorElementType();
+
----------------
->      FalseVal->getOperand(0).getValueType().getVectorElementType();


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5214
+  // so check and use the first operand in those cases
+  auto OperandsAreValid = [&](SDValue &Compared, SDValue &Selected,
+                              SDValue &OtherSelected) {
----------------
Hmm. This is getting complicated.

I would suggest changing this to something more like:
  if (Type == i32)
    return Selected == Compared;
  else
    // Check the opcodes are and/signexted/assertzext/assertsext.

But I don't think that will work very well. For i8/i16 I think we might actually need to be matching:
      t7: i32 = AssertZext t5, ValueType:ch:i8
        t17: i32 = vecreduce_umin t3
      t25: i32 = and t17, Constant:i32<255>
    t27: i32 = select_cc t25, t7, t17, t7, setult:ch
  t21: i32 = and t27, Constant:i32<255>

Including that root "and" (which is the same as the two TrueVal/FalseVal sides of the select_cc being extended). But one loop I tried had no visible way I could see to prove the value needed to be a i16. It might be easier to do this from a combine, not during lowering. So before we have legalized the types.

What do you think about getting i32 working first with this patch, and doing i8/i16 as a followup? That way we can get all the boilerplate out of the way, and it's just this code that produces the ARMISD::VMINVs that we need to work further on.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:5156
 
+SDValue ARMTargetLowering::LowerSelectCCToVectorReduction(SDValue Op, SelectionDAG &DAG) const {
+  EVT VT = Op.getValueType();
----------------
Please clang-format, and possible just make it a static function?


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

https://reviews.llvm.org/D87836