[PATCH] D144927: [GVNHoist] don't hoist callbr users into the callbr's block

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

In D144927#4162156 <https://reviews.llvm.org/D144927#4162156>, @nickdesaulniers wrote:

> In D144927#4160250 <https://reviews.llvm.org/D144927#4160250>, @efriedma wrote:
>
>> If you're going to enable hoisting past a callbr, please add a testcase to ensure we don't hoist a load past a callbr which modifies the memory in question.
>
> Will do. Curiously, `GVNHoist::safeToHoistLdSt` returns `false` even for cases of loads not modified by callbr, so HoistGVN doesn't optimize those cases. FWICT, the call to `GVNHoist::firstInBB` is checking the callbr against itself. It looks like MemorySSA is perhaps claiming that for this input:
>
>   @x = global i32 0
>   @y = global i32 0
>   define i32 @foo4(i1 %z) {
>   entry:
>     callbr void asm "", "=*m,!i"(ptr elementtype(i32) @x)
>             to label %a [label %b]
>   
>   a:                                                ; preds = %entry
>     %0 = load i32, ptr @y, align 4
>     ret i32 %0
>   
>   b:                                                ; preds = %entry
>     %1 = load i32, ptr @y, align 4
>     ret i32 %1
>   }
>
> that `callbr` is a "defining access" for the access of `@y` (IIUC)? That seems incorrect...but I'm unfamiliar with MemorySSA.

MemorySSA is a very conservative wrapper on top of alias analysis. for any global, it is unlikely to be precise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144927