This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading] Dont RAUW condition incorrectly
ClosedPublic

Authored by anna on May 17 2017, 7:29 AM.

Details

Summary

We have a bug when RAUWing the condition if experimental.guard or assumes is a use of that
condition. This is because LazyValueInfo may have used the guards/assumes to identify the
value of the condition. We cannot use circular logic and apply the value back to
the guard/assume and change
the behaviour of the guard/assume.
For now, disable RAUW for conditions and fix the logic as a next
step: https://reviews.llvm.org/D33257

Event Timeline

anna created this revision.May 17 2017, 7:29 AM
anna added inline comments.May 17 2017, 12:49 PM
test/Transforms/JumpThreading/assume.ll
61

I took this test case from Philip's patch: https://reviews.llvm.org/D28276.

I think this test case proves an important point: we *can never* use circular logic to fold assume, because there is no way of knowing whether control unconditionally reaches assume, i.e. the function may never be called.

So, we don't need something like the one below, although this makes it much clearer that it's illegal to fold the assume.

%notnull = icmp ne i32* %array, null
call exit()
llvm.assume(i1 %notnull)
br i1 %notnull, label %normal, label %error
reames added inline comments.May 17 2017, 1:52 PM
lib/Transforms/Scalar/JumpThreading.cpp
838

Er, I don't see why this is a circular logic bug? I though the problem was that we were using a fact from the end of the block to reason about uses of the value before the assume/guard?

anna updated this revision to Diff 99348.May 17 2017, 2:19 PM

updated comments and added test to show that this is more than a circular logic bug.

anna marked an inline comment as done.May 17 2017, 2:19 PM
anna added inline comments.
lib/Transforms/Scalar/JumpThreading.cpp
838

Yes. Bad phrasing on my part. This is *more* than a circular logic bug. I've updated the comment and add test cases to reflect that.

anna marked an inline comment as done.May 17 2017, 3:40 PM

While working on the actual fix for this, I realized the test comments for assumes need to be updated.
Specifically, the semantics for assume according to langref:

The intrinsic allows the optimizer to assume that the provided condition is always true whenever the control flow reaches the intrinsic call.

So, we *can* fold the assume and uses after the assume. What we cannot fold are uses *before* the assume.

anna updated this revision to Diff 99358.May 17 2017, 3:42 PM

Updated the assume test cases.

reames accepted this revision.May 17 2017, 4:20 PM

LGTM

This revision is now accepted and ready to land.May 17 2017, 4:20 PM
This revision was automatically updated to reflect the committed changes.