Merge pull request #99 from frasercrmck/fix-live-mem-info-users

[compiler] Remove assume-like intrinsics used by allocas

modules/compiler/test/lit/passes/barriers-assume-like-intrinsics.ll

@@ -0,0 +1,76 @@
+; Copyright (C) Codeplay Software Limited
+;
+; Licensed under the Apache License, Version 2.0 (the "License") with LLVM
+; Exceptions; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+;     https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+; WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+; License for the specific language governing permissions and limitations
+; under the License.
+;
+; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+; RUN: muxc --passes barriers-pass,verify < %s | FileCheck %s
+
+target triple = "spir64-unknown-unknown"
+target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
+
+define void @lifetime(ptr addrspace(1) %input, ptr addrspace(1) %output) #0 {
+entry:
+; Check that we've removed all assume-like intrinsics that were using the
+; alloca, as they're unsafe to keep around.
+; * The llvm.assume may not hold, though admittedly this is a contrived
+;   example.
+; * It should be self-evident that llvm.lifetime.invariant.* intrinsics no
+;   longer hold.
+; * For llvm.lifetime.* intrinsics, note that %live_gep_addr is also the 'base'
+;   of the live mem info, and so replacing %addr with %live_gep_addr in the
+;   lifetime.start intrinsic essentially tells LLVM that *all* memory access
+;   *based on* the live mem info also start at the same point which we call
+;   lifetime.start. This is not true, as may we wish to access pointers based
+;   on the live mem info beforehand, such as the store to %live_gep_b (which is
+;   inserted before where the lifetime.start would have been).
+; CHECK: define internal i32 @lifetime.mux-barrier-region(
+; CHECK:      %live_gep_addr = getelementptr %lifetime_live_mem_info, ptr %2, i32 0, i32 0
+; CHECK-NEXT: %live_gep_b = getelementptr %lifetime_live_mem_info, ptr %2, i32 0, i32 1
+; CHECK-NEXT: %b = load i32, ptr addrspace(1) %0, align 4
+; CHECK-NEXT: store i32 %b, ptr %live_gep_b, align 4
+; CHECK-NEXT: store i32 4, ptr %live_gep_addr, align 4
+; CHECK-NEXT: ret i32
+  %addr = alloca i32, align 4
+  call void @llvm.assume(i1 true) [ "dereferenceable"(ptr %addr, i64 4) ]
+  %p = call ptr @llvm.invariant.start.p0(i64 4, ptr %addr)
+  %b = load i32, ptr addrspace(1) %input
+  call void @llvm.lifetime.start.p0(i64 4, ptr %addr)
+  call void @llvm.invariant.end.p0(ptr %p, i64 4, ptr %addr)
+  store i32 4, ptr %addr, align 4
+
+  call void @__mux_work_group_barrier(i32 0, i32 1, i32 272)
+
+; After the barrier, next region: check there's no lifetime.end either
+; CHECK: define internal i32 @lifetime.mux-barrier-region.1(
+; CHECK-NOT: call void @llvm.lifetime.end
+; CHECK: ret i32
+  %ld = load i32, ptr %addr, align 4
+  call void @llvm.lifetime.end.p0(i64 4, ptr %addr)
+  %res = add i32 %ld, %b
+  store i32 %res, ptr addrspace(1) %output
+  ret void
+}
+
+declare void @llvm.assume(i1)
+
+declare void @llvm.lifetime.end.p0(i64, ptr nocapture)
+declare void @llvm.lifetime.start.p0(i64, ptr nocapture)
+
+declare ptr @llvm.invariant.start.p0(i64, ptr nocapture)
+declare void @llvm.invariant.end.p0(ptr, i64, ptr nocapture)
+
+; Function Attrs: convergent nounwind
+declare void @__mux_work_group_barrier(i32, i32, i32)
+
+attributes #0 = { norecurse nounwind "mux-kernel"="entry-point" }

modules/compiler/utils/source/barrier_regions.cpp

@@ -1095,7 +1095,7 @@ Function *compiler::utils::Barrier::GenerateNewKernel(BarrierRegion &region) {
     }
   }
 
-  SmallVector<AllocaInst *, 8> allocas_to_remove;
+  SmallVector<Instruction *, 8> allocas_and_intrinsics_to_remove;
 
   // Store only live variables that are defined in this kernel.
   //
@@ -1120,8 +1120,22 @@ Function *compiler::utils::Barrier::GenerateNewKernel(BarrierRegion &region) {
       // Check to see if it is still an alloca after vmap. If not we may
       // have processed it before and no work needs doing as we are using
       // the live variable struct directly.
-      if ((alloca_inst = dyn_cast<AllocaInst>(vmap[alloca_inst]))) {
-        allocas_to_remove.push_back(alloca_inst);
+      if (auto *new_alloca_inst = dyn_cast<AllocaInst>(vmap[alloca_inst])) {
+        allocas_and_intrinsics_to_remove.push_back(new_alloca_inst);
+        // Also remove any assume-like intrinsics that are users of this
+        // alloca. These assumptions may not hold. For example, lifetime
+        // intrinsics are definitely dangerous, as by directly replacing their
+        // alloca operands with the address of the live variable struct, we are
+        // telling LLVM that *all* accesses of the live variable struct also
+        // start/end at that point, which is not true.
+        // Similarly, llvm.assume and llvm.experimental.noalias.scope.decl may
+        // hold for the alloca but not the live variables struct.
+        for (auto *const user : alloca_inst->users()) {
+          if (auto *const intrinsic = dyn_cast<IntrinsicInst>(user);
+              intrinsic && intrinsic->isAssumeLikeIntrinsic()) {
+            allocas_and_intrinsics_to_remove.push_back(intrinsic);
+          }
+        }
         // change the vmap to point to the GEP instead of the original alloca
         vmap[live_var] = live_values.getGEP(live_var);
       }
@@ -1178,9 +1192,10 @@ Function *compiler::utils::Barrier::GenerateNewKernel(BarrierRegion &region) {
     }
   }
 
-  // Remove any allocas that have been replaced by a GEP instruction
-  for (auto alloc : allocas_to_remove) {
-    alloc->eraseFromParent();
+  // Remove any allocas and their dependent intrinsics that have been replaced
+  // by a GEP instruction
+  for (auto *inst : allocas_and_intrinsics_to_remove) {
+    inst->eraseFromParent();
   }
 
   // This needs resetting for the sake of any further new GEPs created