[OpenMP][Opt] Combine `struct ident_t*` during deduplication

[jdoerfert]

Summary

If we deduplicate OpenMP runtime calls we have multiple ident_t* that
represent information like source location. So far, we simply kept the
one used by the replacement call. However, as exposed by PR44893, that
can cause problems if we have stack allocated ident_t objects. While
we need to revisit the use of these as well, it is clear that we
eventually want to merge source location information in some way. With
this patch we add the infrastructure to do so but without doing the
actual merge. Instead we pick a global ident_t from the replaced
calls, if possible, or create a new one with an unknown location
instead.

[JonChesterfield]

Cool. Can we reasonably add the reproducible from 44893 to a regression suite, in addition to these IR tests? It's written in C so would need to be under clang's tests.

This looks like a good fix for the reported bug. I don't see why this failure mode would be unique to ident_t though, at least from re-reading deduplicateRuntimeCalls - is it a feature of the current set of runtime calls that can be deduplicated?

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

191

Probably don't want to drop the private annotation here

[jdoerfert]

Add tests, addressed comments

[jdoerfert]

In D74925#1889185, @JonChesterfield wrote:

Cool. Can we reasonably add the reproducible from 44893 to a regression suite, in addition to these IR tests? It's written in C so would need to be under clang's tests.

Done.

This looks like a good fix for the reported bug. I don't see why this failure mode would be unique to ident_t though, at least from re-reading deduplicateRuntimeCalls - is it a feature of the current set of runtime calls that can be deduplicated?

Good catch. Added enough code to be future proof wrt. miscompiles. Also added an extra test in IR.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

191

I actually do. The new and some of the below functions should be exposed.

[JonChesterfield]

Nice. Thanks!

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

public:
 
   /// Generator for '#omp taskyield'
   ///
   /// \param Loc The location where the taskyield directive was encountered.
   void CreateTaskyield(const LocationDescription &Loc);
 
   ///}
 
-private:
+  /// Return the insertion point used by the underlying IRBuilder.
+  InsertPointTy getInsertionPoint() { return Builder.saveIP(); }
+
   /// Update the internal location to \p Loc.
   bool updateToLocation(const LocationDescription &Loc) {
     Builder.restoreIP(Loc.IP);
     Builder.SetCurrentDebugLocation(Loc.DL);
     return Loc.IP.getBlock() != nullptr;
   }
 
   /// Return the function declaration for the runtime function with \p FnID.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

for (Function *F : SCC) {
         }
       Changed |= deduplicateRuntimeCalls(
           *F, RFIs[OMPRTL___kmpc_global_thread_num], GTIdArg);
     }
 
     return Changed;
   }
 
+  static Value *combinedIdentStruct(Value *Ident0, Value *Ident1,
+                                    bool GlobalOnly) {
+    // TODO: Figure out how to actually combine multiple debug locations. For
+    //       now we just keep the first we find.
+    if (Ident0)
+      return Ident0;
+    if (!GlobalOnly || isa<GlobalValue>(Ident1))
+      return Ident1;
+    return nullptr;
+  }
+
+  /// Return an `struct ident_t*` value that represents the ones used in the
+  /// calls of \p RFI inside of \p F. If \p GlobalOnly is true, we will not
+  /// return a local `struct ident_t*`. For now, if we cannot find a suitable
+  /// return value we create one from scratch. We also do not yet combine
+  /// information, e.g., the source locations, see combinedIdentStruct.
+  Value *getCombinedIdentFromCallUsesIn(RuntimeFunctionInfo &RFI, Function &F,
+                                        bool GlobalOnly) {
+    Value *Ident = nullptr;
+    auto CombineIdentStruct = [&](Use &U, Function &Caller) {
+      CallInst *CI = getCallIfRegularCall(U, &RFI);
+      if (!CI || &F != &Caller)
+        return false;
+      Ident = combinedIdentStruct(Ident, CI->getArgOperand(0),
+                                  /* GlobalOnly */ true);
+      return false;
+    };
+    RFI.foreachUse(CombineIdentStruct);
+
+    if (!Ident) {
+      // The IRBuilder uses the insertion block to get to the module, this is
+      // unfortunate but we work around it for now.
+      if (!OMPBuilder.getInsertionPoint().getBlock())
+        OMPBuilder.updateToLocation(OpenMPIRBuilder::InsertPointTy(
+            &F.getEntryBlock(), F.getEntryBlock().begin()));
+      // Create a fallback location if non was found.
+      // TODO: Use the debug locations of the calls instead.
+      Constant *Loc = OMPBuilder.getOrCreateDefaultSrcLocStr();
+      Ident = OMPBuilder.getOrCreateIdent(Loc);
+    }
+    return Ident;
+  }
+
   /// Try to eliminiate calls of \p RFI in \p F by reusing an existing one or
   /// \p ReplVal if given.
   bool deduplicateRuntimeCalls(Function &F, RuntimeFunctionInfo &RFI,
                                Value *ReplVal = nullptr) {
     auto &Uses = RFI.UsesMap[&F];
     if (Uses.size() + (ReplVal != nullptr) < 2)
       return false;
 
if (!ReplVal) {
           CI->moveBefore(&*F.getEntryBlock().getFirstInsertionPt());
           ReplVal = CI;
           break;
         }
       if (!ReplVal)
         return false;
     }
 
+    // If we use a call as a replacement value we need to make sure the ident is
+    // valid at the new location. For now we just pick a global one, either
+    // existing and used by one of the calls, or created from scratch.
+    if (CallBase *CI = dyn_cast<CallBase>(ReplVal)) {
+      Value *Ident = getCombinedIdentFromCallUsesIn(RFI, F,
+                                                    /* GlobalOnly */ true);
+      CI->setArgOperand(0, Ident);
+    }
+
     bool Changed = false;
     auto ReplaceAndDeleteCB = [&](Use &U, Function &Caller) {
       CallInst *CI = getCallIfRegularCall(U, &RFI);
       if (!CI || CI == ReplVal || &F != &Caller)
         return false;
       assert(CI->getCaller() == &F && "Unexpected call!");
       CGUpdater.removeCallSite(*CI);
       CI->replaceAllUsesWith(ReplVal);

llvm/test/Transforms/OpenMP/gtid.ll

-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
 ; RUN: opt -openmpopt -S < %s | FileCheck %s
 ; RUN: opt -passes=openmpopt -S < %s | FileCheck %s
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 %struct.ident_t = type { i32, i32, i32, i32, i8* }
 
-@0 = private unnamed_addr global %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str, i32 0, i32 0) }, align 8
-@.str = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1
+@0 = private unnamed_addr global %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8
+@1 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str1, i32 0, i32 0) }, align 8
+@2 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str2, i32 0, i32 0) }, align 8
+@.str0 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1
+@.str1 = private unnamed_addr constant [23 x i8] c";file001;loc0001;0;0;;\00", align 1
+@.str2 = private unnamed_addr constant [23 x i8] c";file002;loc0002;0;0;;\00", align 1
+
+; UTC_ARGS: --disable
+; CHECK-DAG: @0 = private unnamed_addr global %struct.ident_t { i32 0, i32 34, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8
+; CHECK-DAG: @1 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str1, i32 0, i32 0) }, align 8
+; CHECK-DAG: @2 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str2, i32 0, i32 0) }, align 8
+; CHECK-DAG: @.str0 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1
+; CHECK-DAG: @.str1 = private unnamed_addr constant [23 x i8] c";file001;loc0001;0;0;;\00", align 1
+; CHECK-DAG: @.str2 = private unnamed_addr constant [23 x i8] c";file002;loc0002;0;0;;\00", align 1
+; CHECK-DAG: @3 = private unnamed_addr global %struct.ident_t { i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds ([23 x i8], [23 x i8]* @.str0, i32 0, i32 0) }, align 8
+; UTC_ARGS: --enable
+
 
 declare i32 @__kmpc_global_thread_num(%struct.ident_t*)
 declare void @useI32(i32)
 
 define void @external(i1 %c) {
 ; CHECK-LABEL: define {{[^@]+}}@external
 ; CHECK-SAME: (i1 [[C:%.*]])
 ; CHECK-NEXT:  entry:
e:
   %c1 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull @0)
   call void @useI32(i32 %c1)
   br label %m
 m:
   %c2 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull @0)
   call void @useI32(i32 %c2)
   ret void
 }
+
+
+define void @local_and_global_gtid_calls() {
+; CHECK-LABEL: define {{[^@]+}}@local_and_global_gtid_calls()
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TID5:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @2)
+; CHECK-NEXT:    [[DOTKMPC_LOC_ADDR:%.*]] = alloca [[STRUCT_IDENT_T:%.*]], align 8
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    ret void
+;
+entry:
+  %.kmpc_loc.addr = alloca %struct.ident_t, align 8
+  %tid0 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr)
+  %tid1 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @1)
+  %tid2 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr)
+  call void @useI32(i32 %tid0)
+  call void @useI32(i32 %tid1)
+  call void @useI32(i32 %tid2)
+  %tid3 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr)
+  %tid4 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @2)
+  %tid5 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr)
+  call void @useI32(i32 %tid3)
+  call void @useI32(i32 %tid4)
+  call void @useI32(i32 %tid5)
+  ret void
+}
+
+define void @local_gtid_calls_only() {
+; CHECK-LABEL: define {{[^@]+}}@local_gtid_calls_only()
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TID5:%.*]] = call i32 @__kmpc_global_thread_num(%struct.ident_t* @3)
+; CHECK-NEXT:    [[DOTKMPC_LOC_ADDR1:%.*]] = alloca [[STRUCT_IDENT_T:%.*]], align 8
+; CHECK-NEXT:    [[DOTKMPC_LOC_ADDR2:%.*]] = alloca [[STRUCT_IDENT_T]], align 8
+; CHECK-NEXT:    [[DOTKMPC_LOC_ADDR3:%.*]] = alloca [[STRUCT_IDENT_T]], align 8
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    call void @useI32(i32 [[TID5]])
+; CHECK-NEXT:    ret void
+;
+entry:
+  %.kmpc_loc.addr1 = alloca %struct.ident_t, align 8
+  %.kmpc_loc.addr2 = alloca %struct.ident_t, align 8
+  %.kmpc_loc.addr3 = alloca %struct.ident_t, align 8
+  %tid0 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr1)
+  %tid1 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr2)
+  %tid2 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr3)
+  call void @useI32(i32 %tid0)
+  call void @useI32(i32 %tid1)
+  call void @useI32(i32 %tid2)
+  %tid3 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr1)
+  %tid4 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr2)
+  %tid5 = call i32 @__kmpc_global_thread_num(%struct.ident_t* %.kmpc_loc.addr3)
+  call void @useI32(i32 %tid3)
+  call void @useI32(i32 %tid4)
+  call void @useI32(i32 %tid5)
+  ret void
+}