This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Try harder to find UB for NSW/NUW instr
Needs ReviewPublic

Authored by christof on Sep 16 2016, 3:09 AM.

Details

Summary

Extended UB analysis of SCEV with the case where an instruction marked as non-wrapping (I) is later used in a latch control expression. In this case, if I holds a poison value, the loop can be made to iterate forever which is undefined behavior.

For wrapping of I to be UB, the latch control expression must execute if I is executed. To proof this, the PostDominatorTree is needed by SCEV. If a post-domination path is found, it is checked that progress from I to the latch control expression is guaranteed. That is, trapping instruction and infinite loops must be absent on that execution path.

This patch is 1/2 to fix PR28429. Known issues:

  • Contains a work around for the LoopPassManager that has trouble with the newly added dependency of the PostDominator pass. This can be approved upon to make the PostDominatorTree pass run less often
  • LoopUnroll uses SCEV and breaks PostDominatorTree, giving an segfault if a function contains multiple loops.

I hope to get a review to see if this work is OK to commit once the known issues listed above are fixed.

Diff Detail

Repository
rL LLVM

Event Timeline

christof updated this revision to Diff 71604.Sep 16 2016, 3:09 AM
christof retitled this revision from to [SCEV] Try harder to find UB for NSW/NUW instr.
christof updated this object.
christof added reviewers: broune, sanjoy, sbaranga.
christof set the repository for this revision to rL LLVM.
christof added a subscriber: llvm-commits.
wmi added a subscriber: wmi.Sep 16 2016, 10:23 AM
sanjoy edited edge metadata.Sep 18 2016, 11:59 PM

Apologies in advance for this off-the-cuff comment with only having skimmed through your patch, let me know if you've already addressed this: infinite loops are not UB in general, they're UB only if the loop has no side effects (i.e. no volatile or atomic operations and no IO).

Apologies in advance for this off-the-cuff comment with only having skimmed through your patch, let me know if you've already addressed this: infinite loops are not UB in general, they're UB only if the loop has no side effects (i.e. no volatile or atomic operations and no IO).

I've used the existing code in SCEV that searches for UB in loops. However, that code does assume infinite loops without side effects to be UB. I guess that this comes from the C++ and C standards.

lib/Analysis/ScalarEvolution.cpp
4938

This (existing, but renamed) function does assume that infinite loops without side effect are also UB. See the long comment on its reasoning.

Let me elaborate a bit more on the UB of infinite loops. There existing code lists 2 cases of infinite loops triggered by poison:

  1. A loop with side effects in it. This is easy as any side effect in the loop is control-dependent on poison.
  2. A loop without side effects in it. The contested one.

The latter case is stated in the existing code now part of isAlwaysLatchControlDependentOnPoison() as being UB. The reasoning behind that could be: No side effect after the loop happens any more, this can be observed.

There is one part of this patch that I am not certain about. I have added code in isAlwaysLatchControlDependentOnPoison to look through some types of phi node. I've annotated it with an in-line comment.

I've noticed the discussion of Sanjoy on llvm-dev that tries to address the defect I attempt to fix here in a different way. However, I would still much appreciate some feedback on this work in the mean time.

lib/Analysis/ScalarEvolution.cpp
4989

I am looking through PHI nodes. I am curious if I missed a case where this is not ok.

I've noticed the discussion of Sanjoy on llvm-dev that tries to address the defect I attempt to fix here in a different way.

+1 to proceeding with this in the mean time. What I started on the mailing list is more of a long term solution and is still very speculative at this point.

lib/Analysis/ScalarEvolution.cpp
4857

If you also check that useI is a TerminatorInst then you'll keep the SmallPtrSet smaller.

4862

Use llvm::any_of. That will also take care of the early exit (that is, you don't need to continue once LatchControlDependentOnPoison is true.

4943

These comments need to be fixed, since IIUC I is no longer necessarily an add recurrence in L.

4995

What about:

loop:
  %iv = phi i32 [ %poison_inst, %entry ], [ 500; %loop ]
  ...
  br (%iv < ...), label %loop, label %leave

?

Since %iv is only poison on the first iteration the proof at the
start of the first function ("If the add recurrence is poison in any
iteration, it is poison on all future iterations (since incrementing
poison yields poison") doesn't hold.

lib/Analysis/ValueTracking.cpp
3603

FYI: you need to follow LLVM coding style here.

lib/Transforms/Utils/LoopUtils.cpp
939

I don't think a loop-pass can not preserve a function pass that it requires. You should get this part reviewed by @chandlerc or @mzolotukhin.

I'm also fairly apprehensive of the compile time impact of computing the post-dom tree since it really isn't used in many other places at this time. At the very least, the burden of proof is on you to show that this patch does not regress compile time.

sanjoy resigned from this revision.Jan 29 2022, 5:36 PM