[PATCH] D158291: [PoC][WIP] Add an AArch64 specific pass for loop idiom recognition

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7481
+    else
+      VSMax = CR.getUpper().getZExtValue();
+
----------------
Is the upper bound here guaranteed to fit into an i64?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7487
+      unsigned MinBW = llvm::bit_width(
+          VSMax * OpVT.getVectorElementCount().getKnownMinValue());
+      EltWidth = llvm::bit_ceil(std::max(MinBW, (unsigned)8));
----------------
This multiply can overflow?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7488
+          VSMax * OpVT.getVectorElementCount().getKnownMinValue());
+      EltWidth = llvm::bit_ceil(std::max(MinBW, (unsigned)8));
+    }
----------------
We never want to increase EltWidth beyond the width of the result, I think?  (If width of the vector doesn't fit into the return value of cttz.elts, is the result poison, or something else?)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7504
+    SDValue And = DAG.getNode(ISD::AND, DL, NewVT, StepVL, Ext);
+    SDValue Max = DAG.getNode(ISD::VECREDUCE_SMAX, DL, NewEltTy, And);
+    SDValue Sub = DAG.getNode(ISD::SUB, DL, NewEltTy, VL, Max);
----------------
Is there some reason to use SMAX instead of UMAX?  It seems to complicate reasoning about the sign bit.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:622
 
+  LPM2.addPass(IndVarSimplifyPass());
+
----------------
Did you really mean to remove LoopIdiomRecognize and replace it with a second run of IndVarSimplify?

I'm not sure why this patch requires messing with the default pass pipeline.


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

https://reviews.llvm.org/D158291