[PATCH] D89382: [SVE][CodeGen] Lower scalable integer vector reductions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16320
+
+  if (!OpVT.isScalableVector() && OpVT.getVectorElementType() != MVT::i1)
+    return SDValue();
----------------
Shouldn't this be `||`?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16337
+  case ISD::VECREDUCE_XOR:
+    SDValue ID =
+        DAG.getTargetConstant(Intrinsic::aarch64_sve_cntp, DL, MVT::i64);
----------------
This will cause a compiler warning if anybody adds a case statement after `ISD::VECREDUCE_XOR`.  Personally I'd just wrap all the case blocks in `{}` and move AArch64CC::CondCode Cond into the blocks that need it.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16341-16343
+    SDValue And = DAG.getNode(ISD::AND, DL, MVT::i64, Cntp,
+                              DAG.getConstant(1, DL, MVT::i64));
+    return DAG.getAnyExtOrTrunc(And, DL, MVT::i32);
----------------
The AND is not required because `ISD::VECREDUCE_XOR` only says the bottom N bits are defined, where N is the size of the operand's vector element.

The call to `getAnyExtOrTrunc` should be using `ReduceOp.getValueType()` and not hardwiring `MVT::i32`.

Given this change `ReduceOp.getValueType()` will be used three times so I'd just have `EVT VT = ReduceOp.getValueType();` at the top of the function, which might have the affect that `AArch64CC::####_ACTIVE` can be used in place and still fit on the line and thus remove the need for `{}` for those entries.


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

https://reviews.llvm.org/D89382