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

[Sanjoy Das]

> 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.

I tried some combinations of the IR and could not get the bug to
manifest (well, manifest visibly) with an -O3.  So I think I will go
with the the '// WARNING' approach and revisit once we've nailed down
the semantics of nsw/nuw arithmetic operations.

-- Sanjoy

>
>  -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