[PATCH] [SCEV][LoopVectorize] Allow ScalarEvolution to make assumptions about overflows

[Sanjoy Das]

I'll make it a point to do a more detailed review this week, but I
have a high level question:

Instead of a separate AssumingScalarEvolution, why not have an
interface like:

  NoOverflowResult ScalarEvolution::proveNoOverflow(SCEVAddRecExpr
*AddRec, FlagType, bool CanAddAssumptions);

NoOverflowResult can be Yes, No or (if CanAddAssumptions is true) a
predicate that, if satisfied, will guarantee AddRec does not overflow
in the FlagType sense.  Then the client can choose if it is profitable
to actually emit the predicate if it is cheap enough relative to expected
payoff.

A much more general interface could be

 Result ScalarEvolution::provePredicate(CmpInst::Pred, SCEV *LHS, SCEV *RHS)

and have it return one of "Yes | No | Guarded (Predicate)"

Then not only can you ask the "sext{addrec} == addrec{sext}" question
to check for no-wrap, but also push more general logic into
ScalarEvolution as needed in the future (i.e. is this add-rec equal to this
other add-rec at any iteration? if not, can I make it so by adding runtime
checks?).

-- Sanjoy

On Mon, Jun 22, 2015 at 9:39 PM, Andrew Trick <atrick at apple.com> wrote:
> Overall, it's great to see something like this working in practice. In the past, several alternatives have been discussed. IIRC the main objection to this approach is that SCEV could make assumptions that are not necessary for the optimization leading to overhead for unnecessary checks.
>
> Given this is a major design decision, it would be good to have feedback from anyone who has done a lot of work in the area (thanks to Hal for the review). What about Sanjoy, Arnold (especially for the vectorizer change), MichaelZ, AdamN ??
>
> I don't see a major problem with this approach. Just a few comments on the implementation...
>
> I'm afraid that iterating over a DenseMap will produce SCEV expressions and IR checks in a nondeterministic order. We usually fix this with a MapVector.
>
> +    AssumptionMapTy::iterator getLoopAssumptionBegin(const Loop *L) {
>
> I don't see AnalyzedLoop being initialized in the ctor:
>
> + AssumingScalarEvolution::AssumingScalarEvolution() :
> +   ScalarEvolution(false, ID), SE(nullptr) {
>
> See Instruction::getModule()
>
> +  Module *M = Loc->getParent()->getParent()->getParent();
>
> On most targets, I think it's more efficient to check the high bits for overflow. e.g.
>
>   if ((a + 0x800000) & 0xffffff000000) overflow
>
> Instead of:
>
> +    if (Signed) {
> +      APInt CmpMinValue = APInt::getSignedMinValue(DstBits).sext(SrcBits);
> +      ConstantInt *CTMin = ConstantInt::get(M->getContext(), CmpMinValue);
> +      Value *MinCheck = OFBuilder.CreateICmp(ICmpInst::ICMP_SLT,
> +                                             TripCount,
> +                                             CTMin);
> +      TripCountCheck = OFBuilder.CreateOr(TripCountCheck, MinCheck);
> +    }
>
>
> http://reviews.llvm.org/D10161
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits