[OpenMP] Fix insertion point for deduplicated runtime call to honor def-use order

[wenlei]

Summary

A recently added OpenMP optimization pass (D69930) tries to deduplicate omp runtime calls by merging the calls and insert a new one in entry block. However the insertion didn't honor def-use chain of parameters, so it can introduce use-before-def. This change fixed the problem by taking def-use into account for finding insertion point of the merged runtime call.

[jdoerfert]

This was reported as PR44893, and will be fixed by https://reviews.llvm.org/D74925.

[wenlei]

@jdoerfert good to know it's being fixed - thanks! I saw that fix is accepted, do you plan to land it now, asking because this broke dozens of internal builds, so we want to resolve it asap - deciding whether to wait for your fix to go in or we need to port over your fix manually.

[jdoerfert]

Fixed in 396b7253944e.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

LLVM_DEBUG(dbgs() << TAG << "Deduplicate " << Uses.size() << " uses of "
                       << (ReplVal ? " with an existing value\n" : "\n")
                       << "\n");
     assert((!ReplVal || (isa<Argument>(ReplVal) &&
                          cast<Argument>(ReplVal)->getParent() == &F)) &&
            "Unexpected replacement value!");
     if (!ReplVal) {
       for (Use *U : Uses)
         if (CallInst *CI = getCallIfRegularCall(*U, &RFI)) {
-          CI->moveBefore(&*F.getEntryBlock().getFirstInsertionPt());
+          Value *Op0 = CI->getOperand(0);
+          Instruction *MovePt = &*F.getEntryBlock().getFirstInsertionPt();
+          if (Instruction *DefInst = dyn_cast<Instruction>(Op0)) {
+            if (DefInst->getParent() != &F.getEntryBlock())
+              break;
+            if (!DefInst->comesBefore(MovePt)) {
+              MovePt = DefInst->getNextNode();
+            }
+          }
+          CI->moveBefore(MovePt);
           ReplVal = CI;
           break;
         }
       if (!ReplVal)
         return false;
     }
 
     bool Changed = false;

llvm/test/Transforms/OpenMP/gtid.ll

 
 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:
-; CHECK-NEXT:    [[C2:%.*]] = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull @0)
+; CHECK-NEXT:    %tmp1 = alloca %struct.ident_t
+; CHECK-NEXT:    [[C2:%.*]] = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull %tmp1)
 ; CHECK-NEXT:    br i1 [[C]], label [[T:%.*]], label [[E:%.*]]
 ; CHECK:       t:
 ; CHECK-NEXT:    call void @internal(i32 [[C2]], i32 [[C2]])
 ; CHECK-NEXT:    call void @useI32(i32 [[C2]])
 ; CHECK-NEXT:    br label [[M:%.*]]
 ; CHECK:       e:
 ; CHECK-NEXT:    call void @internal(i32 [[C2]], i32 [[C2]])
 ; CHECK-NEXT:    call void @useI32(i32 [[C2]])
 ; CHECK-NEXT:    br label [[M]]
 ; CHECK:       m:
 ; CHECK-NEXT:    call void @internal(i32 0, i32 [[C2]])
 ; CHECK-NEXT:    call void @useI32(i32 [[C2]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
+  %tmp1 = alloca %struct.ident_t, align 8
   br i1 %c, label %t, label %e
 t:
-  %c0 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull @0)
+  %c0 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull %tmp1)
   call void @internal(i32 %c0, i32 %c0)
   call void @useI32(i32 %c0)
   br label %m
 e:
-  %c1 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull @0)
+  %c1 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull %tmp1)
   call void @internal(i32 %c1, i32 %c1)
   call void @useI32(i32 %c1)
   br label %m
 m:
-  %c2 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull @0)
+  %c2 = tail call i32 @__kmpc_global_thread_num(%struct.ident_t* nonnull %tmp1)
   call void @internal(i32 0, i32 %c2)
   call void @useI32(i32 %c2)
   ret void
 }
 
 define internal void @internal(i32 %not_gtid, i32 %gtid) {
 ; CHECK-LABEL: define {{[^@]+}}@internal
 ; CHECK-SAME: (i32 [[NOT_GTID:%.*]], i32 [[GTID:%.*]])