Applied remarks

Author: DominikAdamski

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h

@@ -277,6 +277,16 @@ enum class RTLDependenceKindTy {
   DepOmpAllMem = 0x80,
 };
 
+/// A type of worksharing loop construct
+enum class WorksharingLoopType {
+  // Worksharing `for`-loop
+  ForStaticLoop,
+  // Worksharing `distrbute`-loop
+  DistributeStaticLoop,
+  // Worksharing `distrbute parallel for`-loop
+  DistributeForStaticLoop
+};
+
 } // end namespace omp
 
 } // end namespace llvm

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

@@ -439,16 +439,6 @@ class OffloadEntriesInfoManager {
 /// Each OpenMP directive has a corresponding public generator method.
 class OpenMPIRBuilder {
 public:
-  /// A type of worksharing loop construct
-  enum class WorksharingLoopType {
-    // Worksharing `for`-loop
-    ForStaticLoop,
-    // Worksharing `distrbute`-loop
-    DistributeStaticLoop,
-    // Worksharing `distrbute parallel for`-loop
-    DistributeForStaticLoop
-  };
-
   /// Create a new OpenMPIRBuilder operating on the given module \p M. This will
   /// not have an effect on \p M (see initialize)
   OpenMPIRBuilder(Module &M)
@@ -913,8 +903,8 @@ class OpenMPIRBuilder {
   /// Modifies the canonical loop to be a statically-scheduled workshare loop
   /// which is executed on the device
   ///
-  /// This takes a \p LoopInfo representing a canonical loop, such as the one
-  /// created by \p createCanonicalLoop and emits additional instructions to
+  /// This takes a \p CLI representing a canonical loop, such as the one
+  /// created by \see createCanonicalLoop and emits additional instructions to
   /// turn it into a workshare loop. In particular, it calls to an OpenMP
   /// runtime function in the preheader to call OpenMP device rtl function
   /// which handles worksharing of loop body interations.
@@ -930,7 +920,7 @@ class OpenMPIRBuilder {
   /// \returns Point where to insert code after the workshare construct.
   InsertPointTy applyWorkshareLoopTarget(DebugLoc DL, CanonicalLoopInfo *CLI,
                                          InsertPointTy AllocaIP,
-                                         WorksharingLoopType LoopType);
+                                         omp::WorksharingLoopType LoopType);
 
   /// Modifies the canonical loop to be a statically-scheduled workshare loop.
   ///
@@ -1055,7 +1045,8 @@ class OpenMPIRBuilder {
       Value *ChunkSize = nullptr, bool HasSimdModifier = false,
       bool HasMonotonicModifier = false, bool HasNonmonotonicModifier = false,
       bool HasOrderedClause = false,
-      WorksharingLoopType LoopType = WorksharingLoopType::ForStaticLoop);
+      omp::WorksharingLoopType LoopType =
+          omp::WorksharingLoopType::ForStaticLoop);
 
   /// Tile a loop nest.
   ///

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

@@ -2679,27 +2679,27 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::applyStaticChunkedWorkshareLoop(
 // Always interpret integers as unsigned similarly to CanonicalLoopInfo.
 static FunctionCallee
 getKmpcForStaticLoopForType(Type *Ty, OpenMPIRBuilder *OMPBuilder,
-                            OpenMPIRBuilder::WorksharingLoopType LoopType) {
+                            WorksharingLoopType LoopType) {
   unsigned Bitwidth = Ty->getIntegerBitWidth();
   Module &M = OMPBuilder->M;
   switch (LoopType) {
-  case OpenMPIRBuilder::WorksharingLoopType::ForStaticLoop:
+  case WorksharingLoopType::ForStaticLoop:
     if (Bitwidth == 32)
       return OMPBuilder->getOrCreateRuntimeFunction(
           M, omp::RuntimeFunction::OMPRTL___kmpc_for_static_loop_4u);
     if (Bitwidth == 64)
       return OMPBuilder->getOrCreateRuntimeFunction(
           M, omp::RuntimeFunction::OMPRTL___kmpc_for_static_loop_8u);
     break;
-  case OpenMPIRBuilder::WorksharingLoopType::DistributeStaticLoop:
+  case WorksharingLoopType::DistributeStaticLoop:
     if (Bitwidth == 32)
       return OMPBuilder->getOrCreateRuntimeFunction(
           M, omp::RuntimeFunction::OMPRTL___kmpc_distribute_static_loop_4u);
     if (Bitwidth == 64)
       return OMPBuilder->getOrCreateRuntimeFunction(
           M, omp::RuntimeFunction::OMPRTL___kmpc_distribute_static_loop_8u);
     break;
-  case OpenMPIRBuilder::WorksharingLoopType::DistributeForStaticLoop:
+  case WorksharingLoopType::DistributeForStaticLoop:
     if (Bitwidth == 32)
       return OMPBuilder->getOrCreateRuntimeFunction(
           M, omp::RuntimeFunction::OMPRTL___kmpc_distribute_for_static_loop_4u);
@@ -2708,15 +2708,16 @@ getKmpcForStaticLoopForType(Type *Ty, OpenMPIRBuilder *OMPBuilder,
           M, omp::RuntimeFunction::OMPRTL___kmpc_distribute_for_static_loop_8u);
     break;
   }
-  if (Bitwidth != 32 && Bitwidth != 64)
-    llvm_unreachable("unknown OpenMP loop iterator bitwidth");
-  return FunctionCallee();
+  if (Bitwidth != 32 && Bitwidth != 64) {
+    llvm_unreachable("Unknown OpenMP loop iterator bitwidth");
+  }
+  llvm_unreachable("Unknown type of OpenMP worksharing loop");
 }
 
 // Inserts a call to proper OpenMP Device RTL function which handles
 // loop worksharing.
 static void createTargetLoopWorkshareCall(
-    OpenMPIRBuilder *OMPBuilder, OpenMPIRBuilder::WorksharingLoopType LoopType,
+    OpenMPIRBuilder *OMPBuilder, WorksharingLoopType LoopType,
     BasicBlock *InsertBlock, Value *Ident, Value *LoopBodyArg,
     Type *ParallelTaskPtr, Value *TripCount, Function &LoopBodyFn) {
   Type *TripCountTy = TripCount->getType();
@@ -2726,16 +2727,11 @@ static void createTargetLoopWorkshareCall(
       getKmpcForStaticLoopForType(TripCountTy, OMPBuilder, LoopType);
   SmallVector<Value *, 8> RealArgs;
   RealArgs.push_back(Ident);
-  /*loop body func*/
   RealArgs.push_back(Builder.CreateBitCast(&LoopBodyFn, ParallelTaskPtr));
-  /*loop body args*/
   RealArgs.push_back(LoopBodyArg);
-  /*num of iters*/
   RealArgs.push_back(TripCount);
-  if (LoopType == OpenMPIRBuilder::WorksharingLoopType::DistributeStaticLoop) {
-    /*block chunk*/ RealArgs.push_back(TripCountTy->getIntegerBitWidth() == 32
-                                           ? Builder.getInt32(0)
-                                           : Builder.getInt64(0));
+  if (LoopType == WorksharingLoopType::DistributeStaticLoop) {
+    RealArgs.push_back(ConstantInt::get(TripCountTy, 0));
     Builder.CreateCall(RTLFn, RealArgs);
     return;
   }
@@ -2744,17 +2740,12 @@ static void createTargetLoopWorkshareCall(
   Builder.restoreIP({InsertBlock, std::prev(InsertBlock->end())});
   Value *NumThreads = Builder.CreateCall(RTLNumThreads, {});
 
-  /*num of threads*/ RealArgs.push_back(
+  RealArgs.push_back(
       Builder.CreateZExtOrTrunc(NumThreads, TripCountTy, "num.threads.cast"));
-  if (LoopType ==
-      OpenMPIRBuilder::WorksharingLoopType::DistributeForStaticLoop) {
-    /*block chunk*/ RealArgs.push_back(TripCountTy->getIntegerBitWidth() == 32
-                                           ? Builder.getInt32(0)
-                                           : Builder.getInt64(0));
+  RealArgs.push_back(ConstantInt::get(TripCountTy, 0));
+  if (LoopType == WorksharingLoopType::DistributeForStaticLoop) {
+    RealArgs.push_back(ConstantInt::get(TripCountTy, 0));
   }
-  /*thread chunk */ RealArgs.push_back(TripCountTy->getIntegerBitWidth() == 32
-                                           ? Builder.getInt32(1)
-                                           : Builder.getInt64(1));
 
   Builder.CreateCall(RTLFn, RealArgs);
 }
@@ -2764,7 +2755,7 @@ workshareLoopTargetCallback(OpenMPIRBuilder *OMPIRBuilder,
                             CanonicalLoopInfo *CLI, Value *Ident,
                             Function &OutlinedFn, Type *ParallelTaskPtr,
                             const SmallVector<Instruction *, 4> &ToBeDeleted,
-                            OpenMPIRBuilder::WorksharingLoopType LoopType) {
+                            WorksharingLoopType LoopType) {
   IRBuilder<> &Builder = OMPIRBuilder->Builder;
   BasicBlock *Preheader = CLI->getPreheader();
   Value *TripCount = CLI->getTripCount();
@@ -2795,19 +2786,19 @@ workshareLoopTargetCallback(OpenMPIRBuilder *OMPIRBuilder,
   // Find the instruction which corresponds to loop body argument structure
   // and remove the call to loop body function instruction.
   Value *LoopBodyArg;
-  for (auto instIt = Preheader->begin(); instIt != Preheader->end(); ++instIt) {
-    if (CallInst *CallInstruction = dyn_cast<CallInst>(instIt)) {
-      if (CallInstruction->getCalledFunction() == &OutlinedFn) {
-        // Check in case no argument structure has been passed.
-        if (CallInstruction->arg_size() > 1)
-          LoopBodyArg = CallInstruction->getArgOperand(1);
-        else
-          LoopBodyArg = Constant::getNullValue(Builder.getPtrTy());
-        CallInstruction->eraseFromParent();
-        break;
-      }
-    }
-  }
+  User *OutlinedFnUser = OutlinedFn.getUniqueUndroppableUser();
+  assert(OutlinedFnUser &&
+         "Expected unique undroppable user of outlined function");
+  CallInst *OutlinedFnCallInstruction = dyn_cast<CallInst>(OutlinedFnUser);
+  assert(OutlinedFnCallInstruction && "Expected outlined function call");
+  assert((OutlinedFnCallInstruction->getParent() == Preheader) &&
+         "Expected outlined function call to be located in loop preheader");
+  // Check in case no argument structure has been passed.
+  if (OutlinedFnCallInstruction->arg_size() > 1)
+    LoopBodyArg = OutlinedFnCallInstruction->getArgOperand(1);
+  else
+    LoopBodyArg = Constant::getNullValue(Builder.getPtrTy());
+  OutlinedFnCallInstruction->eraseFromParent();
 
   createTargetLoopWorkshareCall(OMPIRBuilder, LoopType, Preheader, Ident,
                                 LoopBodyArg, ParallelTaskPtr, TripCount,
@@ -2818,9 +2809,10 @@ workshareLoopTargetCallback(OpenMPIRBuilder *OMPIRBuilder,
   CLI->invalidate();
 }
 
-OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::applyWorkshareLoopTarget(
-    DebugLoc DL, CanonicalLoopInfo *CLI, InsertPointTy AllocaIP,
-    OpenMPIRBuilder::WorksharingLoopType LoopType) {
+OpenMPIRBuilder::InsertPointTy
+OpenMPIRBuilder::applyWorkshareLoopTarget(DebugLoc DL, CanonicalLoopInfo *CLI,
+                                          InsertPointTy AllocaIP,
+                                          WorksharingLoopType LoopType) {
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(DL, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
@@ -2844,14 +2836,14 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::applyWorkshareLoopTarget(
 
   // Insert new loop counter variable which will be used only in loop
   // body.
-  AllocaInst *newLoopCnt = Builder.CreateAlloca(CLI->getIndVarType(), 0, "");
-  Instruction *newLoopCntLoad =
-      Builder.CreateLoad(CLI->getIndVarType(), newLoopCnt);
+  AllocaInst *NewLoopCnt = Builder.CreateAlloca(CLI->getIndVarType(), 0, "");
+  Instruction *NewLoopCntLoad =
+      Builder.CreateLoad(CLI->getIndVarType(), NewLoopCnt);
   // New loop counter instructions are redundant in the loop preheader when
   // code generation for workshare loop is finshed. That's why mark them as
   // ready for deletion.
-  ToBeDeleted.push_back(newLoopCntLoad);
-  ToBeDeleted.push_back(newLoopCnt);
+  ToBeDeleted.push_back(NewLoopCntLoad);
+  ToBeDeleted.push_back(NewLoopCnt);
 
   // Analyse loop body region. Find all input variables which are used inside
   // loop body region.
@@ -2884,22 +2876,17 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::applyWorkshareLoopTarget(
   // We need to model loop body region as the function f(cnt, loop_arg).
   // That's why we replace loop induction variable by the new counter
   // which will be one of loop body function argument
-  std::vector<User *> Users(CLI->getIndVar()->user_begin(),
-                            CLI->getIndVar()->user_end());
-  for (User *use : Users) {
-    if (Instruction *inst = dyn_cast<Instruction>(use)) {
-      if (ParallelRegionBlockSet.count(inst->getParent())) {
-        inst->replaceUsesOfWith(CLI->getIndVar(), newLoopCntLoad);
+  for (auto Use = CLI->getIndVar()->user_begin();
+       Use != CLI->getIndVar()->user_end(); ++Use) {
+    if (Instruction *Inst = dyn_cast<Instruction>(*Use)) {
+      if (ParallelRegionBlockSet.count(Inst->getParent())) {
+        Inst->replaceUsesOfWith(CLI->getIndVar(), NewLoopCntLoad);
       }
     }
   }
-  Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands);
-  for (Value *Input : Inputs) {
-    // Make sure that loop counter variable is not merged into loop body
-    // function argument structure and it is passed as separate variable
-    if (Input == newLoopCntLoad)
-      OI.ExcludeArgsFromAggregate.push_back(Input);
-  }
+  // Make sure that loop counter variable is not merged into loop body
+  // function argument structure and it is passed as separate variable
+  OI.ExcludeArgsFromAggregate.push_back(NewLoopCntLoad);
 
   // PostOutline CB is invoked when loop body function is outlined and
   // loop body is replaced by call to outlined function. We need to add
@@ -2920,7 +2907,7 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::applyWorkshareLoop(
     bool NeedsBarrier, omp::ScheduleKind SchedKind, Value *ChunkSize,
     bool HasSimdModifier, bool HasMonotonicModifier,
     bool HasNonmonotonicModifier, bool HasOrderedClause,
-    OpenMPIRBuilder::WorksharingLoopType LoopType) {
+    WorksharingLoopType LoopType) {
   if (Config.isTargetDevice())
     return applyWorkshareLoopTarget(DL, CLI, AllocaIP, LoopType);
   OMPScheduleType EffectiveScheduleType = computeOpenMPScheduleType(

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

@@ -2257,26 +2257,30 @@ TEST_F(OpenMPIRBuilderTest, StaticWorkshareLoopTarget) {
 
   IRBuilder<>::InsertPoint AfterIP = OMPBuilder.applyWorkshareLoop(
       DL, CLI, AllocaIP, true, OMP_SCHEDULE_Static, nullptr, false, false,
-      false, false, OpenMPIRBuilder::WorksharingLoopType::ForStaticLoop);
+      false, false, WorksharingLoopType::ForStaticLoop);
   Builder.restoreIP(AfterIP);
   Builder.CreateRetVoid();
 
   OMPBuilder.finalize();
   EXPECT_FALSE(verifyModule(*M, &errs()));
 
   CallInst *WorkshareLoopRuntimeCall = nullptr;
+  int WorkshareLoopRuntimeCallCnt = 0;
   for (auto Inst = Preheader->begin(); Inst != Preheader->end(); ++Inst) {
     CallInst *Call = dyn_cast<CallInst>(Inst);
-    if (Call) {
-      if (Call->getCalledFunction()) {
-        if (Call->getCalledFunction()->getName() ==
-            "__kmpc_for_static_loop_4u") {
-          WorkshareLoopRuntimeCall = Call;
-        }
-      }
+    if (!Call)
+      continue;
+    if (!Call->getCalledFunction())
+      continue;
+
+    if (Call->getCalledFunction()->getName() == "__kmpc_for_static_loop_4u") {
+      WorkshareLoopRuntimeCall = Call;
+      WorkshareLoopRuntimeCallCnt++;
     }
   }
   EXPECT_NE(WorkshareLoopRuntimeCall, nullptr);
+  // Verify that there is only one call to workshare loop function
+  EXPECT_EQ(WorkshareLoopRuntimeCallCnt, 1);
   // Check that pointer to loop body function is passed as second argument
   Value *LoopBodyFuncArg = WorkshareLoopRuntimeCall->getArgOperand(1);
   EXPECT_EQ(Builder.getPtrTy(), LoopBodyFuncArg->getType());