[PATCH] D152366: [LoopVectorize] Allow inner loop runtime checks to be hoisted above an outer loop

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1669
+        OuterExitCount->getType()->isIntegerTy()) {
+      const SCEV *NewHigh = cast<SCEVAddRecExpr>(High)->evaluateAtIteration(
+          OuterExitCount, *Exp.getSE());
----------------
david-arm wrote:
> fhahn wrote:
> > Do we need to check if the AddRecs here don't wrap? If it would wrap, could NewHigh < NewLow?
> That's an interesting point!
> 
> So I've looked into this and in order to even get to this point we must have already called evaluateAtIteration once already to get the original High value. This happens in `RuntimePointerChecking::insert`, which does something similar:
> 
>     const SCEV *Ex = PSE.getBackedgeTakenCount();
> 
>     ScStart = AR->getStart();
>     ScEnd = AR->evaluateAtIteration(Ex, *SE);
> 
> I can't see any existing code that worries about ScEnd being < ScStart here and all I'm doing is performing the same evaluation for the outer loop backedge count. It's quite difficult to follow the whole paper trail of complex code in LoopAccessAnalysis, but it seems that AccessAnalysis::createCheckForAccess only worries about overflow if we failed a dependency check and retry the runtime memory checks.
> 
> Since this is under a flag and not enabled by default, would you be happy for now with me adding a TODO here to investigate whether wrapping really is a problem or not?
Yeah it looks like this is handled a bit inconsistently at the moment. One other potential case that might be missed here is when the outer induction is decreasing. In that case I think the new high and lows would be swapped?

> Since this is under a flag and not enabled by default, would you be happy for now with me adding a TODO here to investigate whether wrapping really is a problem or not?

What is your plan in general to enable the flag by default? I think we want to avoid adding this behind a flag and not working towards enabling it by default in the near term;


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

https://reviews.llvm.org/D152366