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

Thu Oct 15 06:10:04 PDT 2020

paulwalker-arm added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1037-1038
       setOperationAction(ISD::TRUNCATE, VT, Custom);
+      setOperationAction(ISD::VECREDUCE_AND, VT, Custom);
+      setOperationAction(ISD::VECREDUCE_OR, VT, Custom);

----------------
FYI: I guess we'll just promote[1] things like VECREDCUDE_ADD but I think [u/s][min/max] are just variants of AND and OR when it comes to i1 types.

[1] Depending on the cost of `ptest` and the fact `VECREDUCE_AND` requires the extra xor, it might end up better to just promote all i1 base operations.  Eitherway I'm happy with your current approach I'm just making you aware of a potential future change.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16179

+  if (useSVEForFixedLengthVectorVT(SrcVT, OverrideNEON)) {
+    EVT ContainerVT = getContainerForFixedLengthVector(DAG, SrcVT);
----------------
Is OverrideNEON needed here?  I ask because when SrcVT is a fixed length vector the value of OverrideNEON seems irrelevant as by calling this function the only safe action is to pack the vector into a scalable type and thus you may as well just always pass in `true`.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16186-16187
+
+  if (SrcVT.getVectorElementType() == MVT::i1)
+    return LowerPredReductionToSVE(Opcode, ScalarOp, Pg, DAG);

----------------
My preference would be to not have any interdependence between these two lowering functions.  If you agree and considering `LowerPredReductionToSVE` has it's own switch statement, can this be moved into `LowerVECREDUCE ` within the `if (SrcVT.isScalableVector...` block and have `LowerPredReductionToSVE` create its own predicate..

================
Comment at: llvm/test/CodeGen/AArch64/sve-int-reduce.ll:50
+
+define i1 @reduce_and_nxv16i1(<vscale x 16 x i1> %vec) {
+; CHECK-LABEL: reduce_and_nxv16i1:
----------------
This is personal preference so feel free to ignore but considering they go down different code paths and have structurally different output I'd prefer to have the predicate tests within their own ll file.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89382