[PATCH] D30667: GVNHoist: handle basic blocks with UnreachableInst

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: llvm/trunk/lib/Analysis/ValueTracking.cpp:3784
     return false;
+  if (isa<UnreachableInst>(I))
+    return false;
----------------
sanjoy wrote:
> This looks fishy to me, returning `true` from here for `unreachable` should also be fine (i.e. both `true` and `false` are correct answers).  The rule for `unreachable` is that executing it generates undefined behavior, so you're allowed to pretend it never executed, which in turn means the client calling this code should be allowed to pretend that it indeed is "guaranteed" to transfer control to any place in the CFG.
> 
> For instance, if the test case for 32153 would have been:
> 
> ```
> define void @test_command(i32 %c1) {
> entry:
>   switch i32 %c1, label %exit [
>     i32 0, label %sw0
>     i32 1, label %sw1
>   ]
> 
> sw0:
>   store i32 1, i32* @G
>   br label %exit
> 
> sw1:
>   store i32 1, i32* @G
>   br label %exit
> 
> exit:
>   unreachable
> }
> ```
> 
> then hoisting the store to `@G` would have been fine.  The only place path on which `@G` is not stored to has undefined behavior.
> 
> The actual bug is that, as @dberlin said, GVNHoist needs to call `isGuaranteedToTransferExecutionToSuccessor` on every instruction, not just terminators.
Hmmm.
I didn't realize that executing it guarantees undefined behavior, but i guess that makes sense.
IMHO, we should probably be explicit in isGuaranteed* as to whether the answer is true or false, but i'm fine with it being false.
In any case, yes, as mentioned, this should not be used to fix a bug, the real bug is elsewhere.





Repository:
  rL LLVM

https://reviews.llvm.org/D30667