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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers 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.

Ok, a quick read of https://llvm.org/docs/MemorySSA.html and running `opt -S -passes=gvn-hoist,'print<memoryssa>' llvm/test/Transforms/GVNHoist/hoist-call.ll -disable-output` gives:

  MemorySSA for function: foo4
  define i32 @foo4(i1 %z) {
  entry:
  ; 1 = MemoryDef(liveOnEntry)
    callbr void asm "", "=*m,!i"(ptr elementtype(i32) @x)
            to label %a [label %b]
  
  a:                                                ; preds = %entry
  ; MemoryUse(1)
    %0 = load i32, ptr @y, align 4
    ret i32 %0
  
  b:                                                ; preds = %entry
  ; MemoryUse(1)
    %1 = load i32, ptr @y, align 4
    ret i32 %1
  }

Shouldn't `callbr` have it's own dedicated `MemoryDef` that's distinct from `liveOnEntry`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144927