handle some review comments

Author: ergawy

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

@@ -542,6 +542,14 @@ static llvm::omp::ProcBindKind getProcBindKind(omp::ClauseProcBindKind kind) {
 /// with entry block arguments is converted to LLVM. In this case \p blockArgs
 /// are (part of) of the OpenMP region's entry arguments and \p operands are
 /// (part of) of the operands to the OpenMP op containing the region.
+///
+/// This function assumes that \p operands belong to the enclosing op of the
+/// block containing \p blockArgs:
+/// ```
+/// enclosing_op(operands) {
+///   block(blockArgs)
+/// }
+/// ```
 static void forwardArgs(LLVM::ModuleTranslation &moduleTranslation,
                         llvm::ArrayRef<BlockArgument> blockArgs,
                         OperandRange operands) {
@@ -5307,21 +5315,17 @@ template <typename OMPOp>
 static void forwardPrivateArgs(OMPOp ompOp,
                                LLVM::ModuleTranslation &moduleTranslation) {
   auto blockArgIface = cast<omp::BlockArgOpenMPOpInterface>(*ompOp);
-  if (blockArgIface) {
-    forwardArgs(moduleTranslation, blockArgIface.getPrivateBlockArgs(),
-                ompOp.getPrivateVars());
-  }
+  forwardArgs(moduleTranslation, blockArgIface.getPrivateBlockArgs(),
+              ompOp.getPrivateVars());
 }
 
 /// Forwards reduction entry block arguments, \see forwardArgs for more details.
 template <typename OMPOp>
 static void forwardReductionArgs(OMPOp ompOp,
                                  LLVM::ModuleTranslation &moduleTranslation) {
   auto blockArgIface = cast<omp::BlockArgOpenMPOpInterface>(*ompOp);
-  if (blockArgIface) {
-    forwardArgs(moduleTranslation, blockArgIface.getReductionBlockArgs(),
-                ompOp.getReductionVars());
-  }
+  forwardArgs(moduleTranslation, blockArgIface.getReductionBlockArgs(),
+              ompOp.getReductionVars());
 }
 
 static LogicalResult
@@ -5346,8 +5350,8 @@ convertTargetOpsInNest(Operation *op, llvm::IRBuilderBase &builder,
 
           // Non-target ops might nest target-related ops, therefore, we
           // translate them as non-OpenMP scopes. Translating them is needed by
-          // nested target-related ops since they might LLVM values defined in
-          // their parent non-target ops.
+          // nested target-related ops since they might need LLVM values defined
+          // in their parent non-target ops.
           if (isa<omp::OpenMPDialect>(oper->getDialect()) &&
               oper->getParentOfType<LLVM::LLVMFuncOp>() &&
               !oper->getRegions().empty()) {
@@ -5367,6 +5371,8 @@ convertTargetOpsInNest(Operation *op, llvm::IRBuilderBase &builder,
                 });
 
             if (auto loopNest = dyn_cast<omp::LoopNestOp>(oper)) {
+              assert(builder.GetInsertBlock() &&
+                     "No insert block is set for the builder");
               for (auto iv : loopNest.getIVs()) {
                 // Create fake allocas just to maintain IR validity.
                 moduleTranslation.mapValue(
@@ -5376,6 +5382,10 @@ convertTargetOpsInNest(Operation *op, llvm::IRBuilderBase &builder,
             }
 
             for (Region &region : oper->getRegions()) {
+              // Regions are fake in the sense that they are not a truthful
+              // translation of the OpenMP construct being converted (e.g. no
+              // OpenMP runtime calls will be generated). We just need this to
+              // prepare the kernel invocation args.
               auto result = convertOmpOpRegions(
                   region, oper->getName().getStringRef().str() + ".fake.region",
                   builder, moduleTranslation);