[PATCH] ScalarEvolution incorrectly assumes that the start of certain add recurrences don't overflow

[Hal Finkel]

----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Sanjoy Das" <sanjoy at playingwithpointers.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>,
> reviews+D7331+public+6ea524414f9332a6 at reviews.llvm.org
> Sent: Saturday, February 7, 2015 7:46:28 AM
> Subject: Re: [PATCH] ScalarEvolution incorrectly assumes that the start of	certain add recurrences don't overflow
> 
> ----- Original Message -----
> > From: "Sanjoy Das" <sanjoy at playingwithpointers.com>
> > To: reviews+D7331+public+6ea524414f9332a6 at reviews.llvm.org
> > Cc: "David Majnemer" <david.majnemer at gmail.com>, "Hal Finkel"
> > <hfinkel at anl.gov>, "Andrew Trick" <atrick at apple.com>,
> > "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> > Sent: Saturday, February 7, 2015 3:20:23 AM
> > Subject: Re: [PATCH] ScalarEvolution incorrectly assumes that the
> > start of certain add recurrences don't overflow
> > 
> > > 1. For now, let's stick with your strongest interpretation, and
> > > assume the local check is sufficient.
> > > 2. Add a loud WARNING comment explaining that the check is only
> > > correct if we assume the overflow cases of nsw are unreachable.
> > 
> > I'd consider this approach somewhat risky -- the rest of LLVM is
> > allowed to do transforms that break this assumption (i.e. they
> > assume
> > arithmetic is not side-effecting).  Consider a slightly modified
> > fragment of the test case with this patch (differences commented):
> 
> I understand (and thanks for working through the example), however, I
> don't want to pre-suppose here the outcome of the poison semantics
> thread. If we adopt Philip's suggestion, for example, and split nsw
> into 'nsw undef' and 'nsw unreachable', then we'll be able to keep
> this logic in one of those cases. Since removing code is easier than
> adding it, I want to keep the check here for now, and add a warning
> documenting the assumption, and we can revisit it once we resolve
> the poison semantics debate (which will hopefully be soon).

Let me revise this slightly. If this can present as a miscompile using the standard optimization pipeline, then I feel we need to fix that. If it can't, then let's wait on the outcome of the poison semantics discussion.

 -Hal

> 
>  -Hal
> 
> > 
> > define i64 @foo(i32 %start, i32 %low.limit, i32 %high.limit, i32*
> > %f)
> > {
> >  entry:
> >   %postinc.start = add i32 %start, 1
> >   br label %loop
> > 
> >  loop:
> >   %idx = phi i32 [ %start, %entry ], [ %idx.inc, %continue ]
> >   %postinc = phi i32 [ %postinc.start, %entry ], [ %postinc.inc,
> >   %continue ]
> >   %postinc.inc = add nsw i32 %postinc, 1
> >   %postinc.sext = sext i32 %postinc to i64
> >   %break.early = icmp slt i32 %postinc, %low.limit
> >   store volatile i32 %idx, i32* %f ;; something to hold %idx in
> >   place
> >   br i1 %break.early, label %continue, label %early.exit
> > 
> >  continue:
> >   %idx.inc = add nsw i32 %idx, 1
> >   br i1 %break.early, label %loop, label %exit ;; repeated use of
> >   %break.early
> > 
> >  exit:
> >   ret i64 0
> > 
> >  early.exit:
> >   ret i64 %postinc.sext
> > }
> > 
> > For the exact same situation / reason as in the test case,
> > SCEV(%postinc)->getStart() is not nsw.
> > 
> > Now if I run this through 'opt -simplifycfg -loop-rotate' I get
> > 
> > define i64 @foo(i32 %start, i32 %low.limit, i32 %high.limit, i32*
> > %f)
> > {
> > entry:
> >   %postinc.start = add i32 %start, 1
> >   br label %loop
> > 
> > loop:                                             ; preds = %loop,
> > %entry
> >   %idx = phi i32 [ %start, %entry ], [ %idx.inc, %loop ]
> >   %postinc = phi i32 [ %postinc.start, %entry ], [ %postinc.inc,
> >   %loop ]
> >   %postinc.inc = add nsw i32 %postinc, 1
> >   %postinc.sext = sext i32 %postinc to i64
> >   %break.early = icmp slt i32 %postinc, %low.limit
> >   store volatile i32 %idx, i32* %f
> >   %idx.inc = add nsw i32 %idx, 1
> >   br i1 %break.early, label %loop, label %early.exit
> > 
> > early.exit:                                       ; preds = %loop
> >   %postinc.sext.lcssa = phi i64 [ %postinc.sext, %loop ]
> >   ret i64 %postinc.sext.lcssa
> > }
> > 
> > `L->getExitingBlock() == L->getLoopLatch()` is true for the loop
> > and
> > we'll trigger the exact same bug even if we add that additional
> > constraint.  This happens with the optimization removed (i.e. with
> > this present version of the change applied) so there is no circular
> > reasoning here.
> > 
> > (Btw, I've assumed Andy really meant `L->getExitingBlock() ==
> > L->getLoopLatch()` and not `L->getExitBlock() == L->getLoopLatch()`
> > since AFAICT the latter is trivially false -- the exit block is
> > always
> > outside the loop while the latch is always inside).
> > 
> > Actually, pretending that a phi consuming poison is unreachable is
> > also risky since I'd expect loop-rerolling to break that
> > assumption.
> > While this means there is no good answer at this time, pretending
> > that
> > a phi consuming poison is UB has the advantage that scev already
> > assumes this; and we won't be introducing more tricky corner cases.
> > 
> > -- Sanjoy
> > 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory