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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added subscribers: efriedma, sanjoy.
sanjoy added a comment.

I think this patch is correct but unnecessary.  It is papering over an issue in GVNHoist.  @efriedma - what do you think?



================
Comment at: llvm/trunk/lib/Analysis/ValueTracking.cpp:3784
     return false;
+  if (isa<UnreachableInst>(I))
+    return false;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D30667