[PATCH] D33257: [JumpThreading] Dont RAUW condition if guards in block

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:266
+      Intrinsic::getName(Intrinsic::experimental_guard));
+  if (GuardDecl && !GuardDecl->use_empty()) {
+    // Traversing the uses requires checking for `Cond` being used directly in
----------------
sanjoy wrote:
> I don't think this is specific to guards.  For instance, if you have:
> 
> ```
> bb:
>   cond = icmp ...;
>   print(cond);
>   exit();
>   assume(cond);
>   br cond;
> ```
> 
> You can't simplify the `cond` in `print(cond)` to `print(true)`.  I think there are similar issues with `isObjectDereferencedInBlock`.
> 
> I'd instead phrase this problem as:
> 
> We know that at the end of a basic block, a certain condition, `cond`, is true.  The question from what point onwards did that condition become true.  To do that, I suggest walking backwards from the branch, and stopping when you find an instruction for which `isGuaranteedToTransferExecutionToSuccessor` returns false.  All the instructions you enumerate this way can be `replaceUsesOfWith(cond, true)` 'ed.  This approach will miss some cases though, like in:
> 
> ```
> bb:
>   cond = icmp ...;
>   print(cond);
>   cond' = or cond, something_else
>   exit();
>   use(cond')
>   assume(cond);
>   br cond;
> ```
> 
> we won't simplify `cond'`.  If needed, we can come up with a more aggressive substitution scheme to get those cases.
> 
Sanjoy's phrasing is a bit different from this phrasing: do not fold guards and assumes and uses before the guards and assumes.
 
Specifically these 2 examples showcase the differences:
1. Using `isGuaranteedToTransferExecutionToSuccessor`, we will be folding assumes. Folding to true is okay, based on lang ref semantics of `assume`. Also, I think there cannot be a case of folding to `assume(false)`, based on how LVI calculates values.
```
bb:
  cond = icmp ...;
  print(cond); <-- can fold this, and we do so.
  assume(cond); <-- can fold this, and we do so.
  br cond;
```
If there was an exit() between print and assume, we cannot fold the `print`. 

2. In this test case below, we unfortunately do not fold either uses because exit is a throwing call, and `isGuaranteedToTransferExecutionToSuccessor` won't allow to fold.
```
bb:
  cond = icmp ...
  assume(cond) <-- can fold this, but not able to.
  print(cond) <-- can fold this, but not able to.
  exit() <-- throws
  br cond
```
I think either phrasing has it's pros and cons, but both are conservatively correct. Thoughts? 


https://reviews.llvm.org/D33257