[PATCH] D136201: [InstCombine] Handle PHI nodes when eliminating constant memcpy

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Just took another look at this. I believe the change to isOnlyCopiedFromConstantMemory() is fine, but the changes to the PointerReplacer are not. Consider these test cases:

  @g1 = constant [32 x i8] zeroinitializer
  @g2 = addrspace(1) constant [32 x i8] zeroinitializer
  
  define i32 @test1(i1 %c, ptr %ptr) {
  entry:
    %a = alloca [32 x i8]
    call void @llvm.memcpy.p0.p0.i64(ptr %a, ptr @g1, i64 32, i1 false)
    br i1 %c, label %if, label %join
  
  if:
    br label %join
  
  join:
    %phi = phi ptr [ %a, %if ], [ %ptr, %entry ]
    %v = load i32, ptr %phi
    ret i32 %v
  }
  
  define i32 @test2(i1 %c, ptr %ptr) {
  entry:
    %a = alloca [32 x i8]
    call void @llvm.memcpy.p0.p1.i64(ptr %a, ptr addrspace(1) @g2, i64 32, i1 false)
    br i1 %c, label %if, label %join
  
  if:
    br label %join
  
  join:
    %phi = phi ptr [ %a, %if ], [ %ptr, %entry ]
    %v = load i32, ptr %phi
    ret i32 %v
  }
  
  declare void @llvm.memcpy.p0.p0.i64(ptr, ptr, i64, i1)
  declare void @llvm.memcpy.p0.p1.i64(ptr, ptr addrspace(1), i64, i1)

The first one works, the second one (using PointerReplacer) will cause an assertion failure. The problem is that once we start to handle phis (or selects), additional operands become involved that might not be rooted in the alloca at all. To handle such cases, we need to make sure that all the phi argument are based on the alloca.

I'd personally recommend to split this patch in two parts. First do only the isOnlyCopiedFromConstantMemory() change (which is trivially correct), and then a separate one to deal with the more complex case of address space conversions.

While not strictly related to the patch, we probably need a limit on number of visited users in isOnlyCopiedFromConstantMemory() -- it's a pre-existing problem, but once we start looking through phis, it becomes more likely that we start looking through large use graphs.



================
Comment at: llvm/test/Transforms/InstCombine/replace-alloca-phi.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=instcombine -S -o - %s | FileCheck %s
+
----------------
Please use opaque pointers for new tests (`[32 x i8] addrspace(4)*` -> `ptr addrspace(4)` etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136201