[PATCH] D65884: [ARM] MVE Tail Predication

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Hi Sam, nice one!

A first scan from my side, just some questions and nits.

> Further work needs to be done to finally produce a true tail predicated loop.

Can you elaborate a little bit on this? I guess you mean instruction selection patterns to finally produce code? Can this work be committed without that being ready? I guess the question rephrased is: can you outline the plan?

You nicely state assumptions in the description, e.g.:

> Because only the loads and stores are predicated, in both the LLVM IR and MIR level, we will restrict support to only lane-wise operations ..
>  ..
>  Another restriction, specific to MVE, is that all the vector instructions need operate on the same number of elements.

I think it would be good to add that to the code too as comments.



================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:1
+//===- MVETailPredication.cpp - MVE Tail Predication ----------------------===//
+//
----------------
nit: MVETailPredication.cpp -> Thumb2TailPredication.cpp?


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:19
+/// code generation. Once the vectorizer has produced a masked loop, there's a
+/// couple of final forms that the loop can generated as:
+/// - A tail-predicated loop, with implicit predication.
----------------
nit: typo "can generate as"?


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:292
+
+bool TPCandidate::IsPredicatedVectorLoop() {
+  // Check that the loop contains at least one masked load/store intrinsic.
----------------
Just wondering if this utility function is interesting and generic enough to be moved to e.g. LoopInfo.


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:303
+        unsigned ElementWidth = VecTy->getScalarSizeInBits();
+        if (Lanes * ElementWidth != 128 || Lanes == 128)
+          return false;
----------------
nit: magic constant?


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:328
+  // TODO: Support constant trip counts.
+
+  auto VisitAdd = [&](const SCEVAddExpr *S) -> const SCEVMulExpr* {
----------------
nit: newline


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:369
+  const SCEV *Elems = nullptr;
+  if (auto *TC = dyn_cast<SCEVAddExpr>(TripCountSE)) {
+    if (auto *Div = dyn_cast<SCEVUDivExpr>(TC->getOperand(1))) {
----------------
nit: this nested-if looks a bit funny, perhaps just getting rid of the brackets helps


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

https://reviews.llvm.org/D65884