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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVETailPredication.cpp:374
   auto *BTC = SE->getSCEV(BackedgeTakenCount);
+  auto *MaxBTC = SE->getConstantMaxBackedgeTakenCount(L);
+
----------------
efriedma wrote:
> 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.
Ah, sorry about this. Don't know what I was thinking...somehow had myself convinced, but it's obviously not really what we need.

I fully agree about your high-level remark. This whole exercise has shown the limitations of our current approach, i.e. the complex analysis required, which we can hopefully avoid by changing the intrinsic/semantics. So, I am going to pursue that direction, and will soon propose something for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85737