[PATCH] D85737: ARM][MVE] tail-predication: overflow checks for backedge taken count

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVETailPredication.cpp:374
   auto *BTC = SE->getSCEV(BackedgeTakenCount);
+  auto *MaxBTC = SE->getConstantMaxBackedgeTakenCount(L);
+
----------------
efriedma wrote:
> SjoerdMeijer wrote:
> > efriedma wrote:
> > > I'm forgetting, do we have a check somewhere that BackedgeTakenCount is actually the backedge-taken count of the loop?
> > Good point. I thought we had one, but we have similar checks for the IV, which is done in step 3) below on line 464. Will address this in a follow up.
> I think I messed up reviewing this.  Took me a bit of time to remember what's going on here.
> 
> `SE->getConstantMaxBackedgeTakenCount()` is the backedge-taken count of the loop.  BTC is the number of elements the loop processes, minus one.  If you want to ensure BTC + 1 doesn't overflow, getConstantMaxBackedgeTakenCount() doesn't actually help, unless you prove some connection between the two values.  The code currently in IsSafeActiveMask does not try to prove that connection.
On a higher-level note, I think this work has shown that trying to force this to work is really complicated, and it might make sense to change the vectorizer to generate something that's easier to analyze.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85737