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

I just tested the updated patch, and this case still gets miscompiled:

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

The memcpy gets removed, even though it might be writing to the `%ptr` argument rather than the `%a` alloca.



================
Comment at: llvm/test/Transforms/InstCombine/replace-alloca-phi.ll:3
+; RUN: opt -passes=instcombine -S -o - %s | FileCheck %s
+
+target triple="amdgcn-amd-amdhsa"
----------------
gandhi21299 wrote:
> arsenm wrote:
> > gandhi21299 wrote:
> > > arsenm wrote:
> > > > arsenm wrote:
> > > > > Drop the triple, this just needs target datalayout ="A5". Otherwise this will fail without amdgpu built 
> > > > Probably should do "p5-32-A5"
> > > Is that a complete specification? I am getting the following error: 
> > > 
> > > `error: Missing size specification for pointer in datalayout string`
> > > 
> > target datalayout = "p5:32:32-A5"
> > 
> `isOnlyCopiedFromConstantMemory` seems to depend on AMDGPUAliasAnalysis results. It returns false for the tests I added when I replace the triple string with the datalayout you suggested.
> 
> cc @bcahoon 
You should be able to avoid making this AMDGPU specific by copying from a constant global, instead of copying from a special AMDGPU address space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136201