[PATCH] D11212: [SCEV] Apply NSW and NUW flags via poison value analysis

[Bjarke Hammersholt Roune]

broune added a comment.

In http://reviews.llvm.org/D11212#206138, @hfinkel wrote:

> In http://reviews.llvm.org/D11212#206104, @broune wrote:
>
> > Added and used isGuaranteedToTransferExecutionToSuccessor (is there a better name?). Also slight improvement to comments.
> >
> > I checked all the instructions in the langref to see if any others might also not terminate. All I found is that while the langref doesn't explicitly say so, some atomics like atomicrmw do not necessarily terminate if another thread keeps interfering. Looking at the C++14 standard, some thread is guaranteed to make progress but I could not find a statement that any given thread is guaranteed to make progress, so I made isGuaranteedToTransferExecutionToSuccessor conservative on that point.
>
>
> I think we'll end up needing some additional attribute to let us get the C++ semantics at the IR level, and we do want them, but we also need to support languages like Java where infinite loops are (sadly) well defined. Regarding C++, 1.10p27 says:
>
>   The implementation may assume that any thread will eventually do one of the following:
>     terminate
>     make a call to a library I/O function
>     access or modify a volatile object, or
>     perform a synchronization operation or an atomic operation
>   
>    [Note: This is intended to allow compiler transformations such as removal of empty loops, even
>     when termination cannot be proven. — end note ]
>   
>
> And, thus, when we can assume C++ semantics, any thread is guaranteed to make progress, or call some external function, or access a volatile/atomic variable.


>From the LLVM atomics guide, we have a pass AtomicExpandPass that e.g. can expand atomicrmw into a loop with compare-exchange. There may be a reason that such a loop always terminates, but I'm not aware of one. The expanded loop does meet the requirement that it will continually perform an atomic operation (just not successfully). If that isn't guaranteed to terminate, and AtomicExpandPass is correct in choosing that implementation for atomicrmw, then it's not clear to me that we can assume that atomicrmw terminates.

It may be that we can assume that e.g. atomicrmw always terminates, I just haven't so far been able to convince myself of that, so I decided to be conservative. I also haven't looked into the Java semantics. If you're confident that it's not an issue, I'll go with that.


================
Comment at: lib/Analysis/ValueTracking.cpp:3369-3370
@@ +3368,4 @@
+  case Instruction::Shl: {
+    // Left shift *by* a poison value is undefined behavior, so we can assume
+    // that that does not happen.
+    //
----------------
majnemer wrote:
> Left shift by poison is poison, not UB.
You're right, thank you. From the langref on shl:

"If op2 is (statically or dynamically) equal to or larger than the number of bits in op1, the result is undefined."

I had read that as undefined behavior, but it only says undefined, which I'm thinking just means undef. That 0 << poison would be poison makes sense to me, so I'll go with that if no one objects.

================
Comment at: lib/Analysis/ValueTracking.cpp:3418-3419
@@ +3417,4 @@
+
+  default:
+    return false;
+  }
----------------
majnemer wrote:
> Call? Invoke?
I'm not sure what the question is. Are you suggesting that there are some intrinsics that it would be good to handle? This function is conservative, so returning false for Call and Invoke is correct.


http://reviews.llvm.org/D11212