fix ClusterConstants (#1213)

When a constant in a phi node, we don't want to create a gep before it. Instead create a gep in the basicblock of the incoming value of the PHI node. Ref #1208
google · Sep 5, 2023 · 96f43a6 · 96f43a6
1 parent fec1967
commit 96f43a6
Show file tree

Hide file tree

Showing 3 changed files with 111 additions and 20 deletions.
diff --git a/lib/ClusterConstants.cpp b/lib/ClusterConstants.cpp
@@ -150,28 +150,51 @@ clspv::ClusterModuleScopeConstantVars::run(Module &M, ModuleAnalysisManager &) {
         } else if (auto *inst = dyn_cast<Instruction>(user)) {
           unsigned index = initializers.idFor(GV->getInitializer()) - 1;
 
-          if (clspv::Option::PhysicalStorageBuffers()) {
-            auto *bb = inst->getParent();
-            auto *clustered_ptr_ty = clspv::GetPushConstantType(
-                M, clspv::PushConstant::ModuleConstantsPointer);
-            auto *ptr_to_ptr = clspv::GetPushConstantPointer(
-                bb, clspv::PushConstant::ModuleConstantsPointer);
-            Value *indices[] = {zero};
-            auto *ptr_gep = Builder.CreateInBoundsGEP(clustered_ptr_ty,
-                                                      ptr_to_ptr, indices);
-            auto *clustered_ptr_val =
-                new LoadInst(clustered_ptr_ty, ptr_gep, "", inst);
-            auto *clustered_ptr =
-                CastInst::Create(Instruction::CastOps::IntToPtr,
-                                  clustered_ptr_val, ptr_type, "", inst);
-            Instruction *gep = GetElementPtrInst::CreateInBounds(
-                type, clustered_ptr, {zero, Builder.getInt32(index)}, "",
-                inst);
-            user->replaceUsesOfWith(GV, gep);
+          auto getTypeAndPtr = [&clustered_gv, &M, &Builder, &type, &ptr_type,
+                                &zero](Type *&PointeeType, Value *&Ptr,
+                                       Instruction *InsertBefore) {
+            if (clspv::Option::PhysicalStorageBuffers()) {
+              auto *bb = InsertBefore->getParent();
+              auto *clustered_ptr_ty = clspv::GetPushConstantType(
+                  M, clspv::PushConstant::ModuleConstantsPointer);
+              auto *ptr_to_ptr = clspv::GetPushConstantPointer(
+                  bb, clspv::PushConstant::ModuleConstantsPointer);
+              Value *indices[] = {zero};
+              auto *ptr_gep = Builder.CreateInBoundsGEP(clustered_ptr_ty,
+                                                        ptr_to_ptr, indices);
+              auto *clustered_ptr_val =
+                  new LoadInst(clustered_ptr_ty, ptr_gep, "", InsertBefore);
+              auto *clustered_ptr = CastInst::Create(
+                  Instruction::CastOps::IntToPtr, clustered_ptr_val, ptr_type,
+                  "", InsertBefore);
+              PointeeType = type;
+              Ptr = clustered_ptr;
+            } else {
+              PointeeType = clustered_gv->getValueType();
+              Ptr = clustered_gv;
+            }
+          };
+
+          if (auto phi = dyn_cast<PHINode>(inst)) {
+            for (unsigned i = 0; i < phi->getNumIncomingValues(); i++) {
+              if (phi->getIncomingValue(i) != GV) {
+                continue;
+              }
+              auto InsertBefore = phi->getIncomingBlock(i)->getFirstNonPHI();
+              Type *PointeeType;
+              Value *Ptr;
+              getTypeAndPtr(PointeeType, Ptr, InsertBefore);
+              Instruction *gep = GetElementPtrInst::CreateInBounds(
+                  PointeeType, Ptr, {zero, Builder.getInt32(index)}, "",
+                  InsertBefore);
+              phi->setIncomingValue(i, gep);
+            }
           } else {
+            Type *PointeeType;
+            Value *Ptr;
+            getTypeAndPtr(PointeeType, Ptr, inst);
             Instruction *gep = GetElementPtrInst::CreateInBounds(
-                clustered_gv->getValueType(), clustered_gv,
-                {zero, Builder.getInt32(index)}, "", inst);
+                PointeeType, Ptr, {zero, Builder.getInt32(index)}, "", inst);
             user->replaceUsesOfWith(GV, gep);
           }
         } else {

diff --git a/test/NormalizeGlobalVariables/cluster_constants_phi.ll b/test/NormalizeGlobalVariables/cluster_constants_phi.ll
@@ -0,0 +1,30 @@
+; RUN: clspv-opt %s -o %t.ll --passes=cluster-constants
+; RUN: FileCheck %s < %t.ll
+
+; CHECK: entry:
+; CHECK:   [[gep_entry:%[^ ]+]] = getelementptr inbounds { <{ [4 x i32] }> }, ptr addrspace(2) @clspv.clustered_constants, i32 0, i32 0
+; CHECK:   br i1 %test, label %true, label %false
+; CHECK: true:
+; CHECK:   [[gep_true:%[^ ]+]] = getelementptr inbounds { <{ [4 x i32] }> }, ptr addrspace(2) @clspv.clustered_constants, i32 0, i32 0
+; CHECK:   br i1 %test2, label %exit, label %false
+; CHECK: false:
+; CHECK:   %phi = phi ptr addrspace(2) [ [[gep_entry]], %entry ], [ [[gep_true]], %true ]
+
+
+target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
+target triple = "spir-unknown-unknown"
+
+@data = addrspace(2) constant <{ [4 x i32] }> <{ [ 4 x i32 ]  zeroinitializer }>, align 4
+
+; Function Attrs: convergent nounwind
+define spir_kernel void @foo(i1 %test, i1 %test2) {
+entry:
+  br i1 %test, label %true, label %false
+true:
+  br i1 %test2, label %exit, label %false
+false:
+  %phi = phi ptr addrspace(2) [ @data, %entry ], [ @data, %true]
+  br label %exit
+exit:
+  ret void
+}
diff --git a/test/NormalizeGlobalVariables/cluster_constants_phi_physical_storage_buffers.ll b/test/NormalizeGlobalVariables/cluster_constants_phi_physical_storage_buffers.ll
@@ -0,0 +1,38 @@
+; RUN: clspv-opt %s -o %t.ll --passes=cluster-constants --physical-storage-buffers
+; RUN: FileCheck %s < %t.ll
+
+; CHECK: entry:
+; CHECK:   [[load:%[^ ]+]] = load i64, ptr addrspace(9) @__push_constants, align 8
+; CHECK:   [[ptr:%[^ ]+]] = inttoptr i64 [[load]] to ptr addrspace(2)
+; CHECK:   [[gep_entry:%[^ ]+]] = getelementptr inbounds { <{ [4 x i32] }> }, ptr addrspace(2) [[ptr]], i32 0, i32 0
+; CHECK:   br i1 %test, label %true, label %false
+; CHECK: true:
+; CHECK:   [[load:%[^ ]+]] = load i64, ptr addrspace(9) @__push_constants, align 8
+; CHECK:   [[ptr:%[^ ]+]] = inttoptr i64 [[load]] to ptr addrspace(2)
+; CHECK:   [[gep_true:%[^ ]+]] = getelementptr inbounds { <{ [4 x i32] }> }, ptr addrspace(2) [[ptr]], i32 0, i32 0
+; CHECK:   br i1 %test2, label %exit, label %false
+; CHECK: false:
+; CHECK:   %phi = phi ptr addrspace(2) [ [[gep_entry]], %entry ], [ [[gep_true]], %true ]
+
+
+target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
+target triple = "spir64-unknown-unknown"
+
+%0 = type { i64 }
+@data = addrspace(2) constant <{ [4 x i32] }> <{ [ 4 x i32 ]  zeroinitializer }>, align 4
+@__push_constants = addrspace(9) global %0 zeroinitializer, !push_constants !0
+
+; Function Attrs: convergent nounwind
+define spir_kernel void @foo(i1 %test, i1 %test2) {
+entry:
+  br i1 %test, label %true, label %false
+true:
+  br i1 %test2, label %exit, label %false
+false:
+  %phi = phi ptr addrspace(2) [ @data, %entry ], [ @data, %true]
+  br label %exit
+exit:
+  ret void
+}
+
+!0 = !{i32 9}