Page MenuHomePhabricator

[CVP] prevent propagating poison when substituting edge values into a phi (PR43802)
ClosedPublic

Authored by spatel on Fri, Oct 25, 12:12 PM.

Details

Summary

This phi simplification transform was added with:
D45448

However as shown in PR43802:
https://bugs.llvm.org/show_bug.cgi?id=43802

...we must be careful not to propagate poison when we do the substitution. There might be some more complicated analysis possible to retain the overflow flag, but it should always be safe and easy to drop flags (we have similar behavior in instcombine and other passes).

Diff Detail

Event Timeline

spatel created this revision.Fri, Oct 25, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Oct 25, 12:12 PM
nikic added inline comments.Fri, Oct 25, 12:24 PM
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
201

I don't think this is sufficient, as poison might come from an earlier instruction CommonValue depends on.

nikic added inline comments.Fri, Oct 25, 2:02 PM
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
201

After looking at the getEdgeValueLocal() implementation, it looks like this should be sufficient after all. The reasoning along the lines of "What value does Y=f(X) given X==C" is only performed for direct users of X.

nikic added inline comments.Fri, Oct 25, 2:07 PM
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
201

If any flags are dropped, processBinOp() should be called here to attempt to re-infer them.

lebedev.ri marked an inline comment as done.Fri, Oct 25, 2:13 PM
lebedev.ri added inline comments.
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
201

I think it may be good to migrate to worklist-based solution.
I'm guessing rerunning CVP until done would come with double the time impact and little to no benefits.

nikic added a comment.Sat, Oct 26, 9:17 AM

I initially thought that there might be a cache invalidation problem here, because we recently started to take nowrap flags into account when computing ranges, and we might end up reasoning based on the no longer present nowrap flags. In particular I had something like this in mind:

efine i1 @test(i32 %arg) {
entry:
  %add = add nuw i32 %arg, 1
  %cmp = icmp eq i32 %arg, -1
  br i1 %cmp, label %bb2, label %bb3

bb2:
  br label %bb3

bb3:
  %r = phi i32 [ 0, %bb2 ], [ %add, %entry ]
  %c = icmp eq i32 %r, 0
  ret i1 %c
}

However, it seems like the same LVI analysis that determines the value of %add on the entry edge here also results in the range of %add in bb3 to be calculated as the full range (rather than the full range without zero), so it ends up working out in the end.

This all seems pretty fragile and there might be issues in the future if LVI gets more powerful, but I guess that for now this is fine.

This all seems pretty fragile and there might be issues in the future if LVI gets more powerful, but I guess that for now this is fine.

I agree, and thanks for looking further into this. I'll leave a TODO comment about this as well as trying to re-infer the flags.

spatel accepted this revision.Mon, Oct 28, 5:57 AM

(marking as accepted based on earlier LGTM)

This revision is now accepted and ready to land.Mon, Oct 28, 5:57 AM
This revision was automatically updated to reflect the committed changes.