[PATCH] D22644: [GVNHoist] Don't clone allocas (PR28606)

[David Majnemer via llvm-commits]

majnemer requested changes to this revision.
majnemer added a reviewer: majnemer.
majnemer added a comment.
This revision now requires changes to proceed.

This value cloning is still quite dangerous...

Consider:

  define void @test(i1 %b, i8** %y, i8** %z) {
  entry:
    %x = load volatile i8*, i8** %z
    br i1 %b, label %true, label %false
  
  true:
    %p = getelementptr inbounds i8*, i8** %y, i32 0
    store i8* %x, i8** %p, align 4
    br label %exit
  
  false:
    %p2 = getelementptr inbounds i8*, i8** %y, i32 0
    store i8* %x, i8** %p2, align 4
    br label %exit
  
  exit:
    ret void
  }

gvn-hoist will make this:

  define void @test(i1 %b, i8** %y, i8** %z) {
  entry:
    %x = load volatile i8*, i8** %z, align 4
    %0 = load volatile i8*, i8** %z, align 4
    store i8* %0, i8** %y, align 4
    br i1 %b, label %true, label %false
  
  true:                                             ; preds = %entry
    br label %exit
  
  false:                                            ; preds = %entry
    br label %exit
  
  exit:                                             ; preds = %false, %true
    ret void
  }

This is bad, we just cloned a volatile instruction.

To be honest, I'm not entirely sure why we must clone the value because we've already checked to see that it is available at the hoist point...


https://reviews.llvm.org/D22644