[PATCH] D71799: [Attributor] AAUndefinedBehavior: Check for branches on undef value.

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added a comment.

In D71799#1796601 <https://reviews.llvm.org/D71799#1796601>, @baziotis wrote:

> So, I tried to make a reduced test case that fails:
>
>   define void @fails() {
>   entry:
>     br i1 undef, label %end, label %end
>  
>   end:
>     %phi = phi i32* [ null, %entry ], [ null, %entry ]
>     %a = load i32, i32* %phi, align 4
>     ret void
>   }
>
>
> It's based on the test case `IPConstantProp/PR26044.ll`, which also fails. The interesting things are:
>
> 1. If we remove the load, it doesn't fail.
> 2. If we remove the phi it doesn't fail (that's also true for `PR26044.ll`).
> 3. As noted yesterday, if we remove the `changeToUnreachable` in `AAUndefinedBehavior::manifest()`, it doesn't fail (all the test cases basically fail because of this part).
> 4. Also, that `AAIsDead` seems to have a problem with it. My guess is that the fact that we change a branch instruction to unreachable means that there's one less predecessor for the then and else blocks of the branch. If one of these 2 hasn't been converted to unreachable and contains a `phi`, then we have problems as now the BB has one less predecessor than those listed in the phi.


I think the problem here is that you are calling `changeToUnrechable` in `manifest`. This might cause unpredictable errors. 
So you should cache instructions to be changed to `unreachable` and call `changeToUnrechable` after manifest.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5570-5571
     }
     for (Instruction *I : UnreachablesToInsert)
       changeToUnreachable(I, /* UseLLVMTrap */ false);
     for (Instruction *I : TerminatorsToFold)
----------------
Here


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71799/new/

https://reviews.llvm.org/D71799