Address more review comments.

Author: jsjodin

flang/lib/Lower/OpenMP/ClauseProcessor.cpp

@@ -529,15 +529,9 @@ bool ClauseProcessor::processProcBind(
 
 bool ClauseProcessor::processTileSizes(
     lower::pft::Evaluation &eval, mlir::omp::LoopNestOperands &result) const {
-  bool found = false;
-  llvm::SmallVector<int64_t> sizeValues;
   auto *ompCons{eval.getIf<parser::OpenMPConstruct>()};
-  collectTileSizesFromOpenMPConstruct(ompCons, sizeValues, semaCtx);
-  if (sizeValues.size() > 0) {
-    found = true;
-    result.tileSizes = sizeValues;
-  }
-  return found;
+  collectTileSizesFromOpenMPConstruct(ompCons, result.tileSizes, semaCtx);
+  return !result.tileSizes.empty();
 }
 
 bool ClauseProcessor::processSafelen(

flang/lib/Lower/OpenMP/Utils.cpp

@@ -599,7 +599,7 @@ static void convertLoopBounds(lower::AbstractConverter &converter,
 }
 
 /// Populates the sizes vector with values if the given OpenMPConstruct
-/// Contains a loop construct with an inner tiling construct.
+/// contains a loop construct with an inner tiling construct.
 void collectTileSizesFromOpenMPConstruct(
     const parser::OpenMPConstruct *ompCons,
     llvm::SmallVectorImpl<int64_t> &tileSizes, SemanticsContext &semaCtx) {
@@ -632,6 +632,7 @@ void collectTileSizesFromOpenMPConstruct(
               if (const auto v{EvaluateInt64(semaCtx, tval)})
                 tileSizes.push_back(*v);
             }
+            break;
           }
       }
     }
@@ -685,15 +686,14 @@ int64_t collectLoopRelatedInfo(
             if (const auto tclause{
                     std::get_if<parser::OmpClause::Sizes>(&clause.u)}) {
               sizesLengthValue = tclause->v.size();
+              break;
             }
         }
       }
     }
   }
 
-  collapseValue = collapseValue - sizesLengthValue;
-  collapseValue =
-      collapseValue < sizesLengthValue ? sizesLengthValue : collapseValue;
+  collapseValue = std::max(collapseValue, sizesLengthValue);
   std::size_t loopVarTypeSize = 0;
   do {
     lower::pft::Evaluation *doLoop =

mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td

@@ -1340,22 +1340,6 @@ class OpenMP_SimdlenClauseSkip<
 
 def OpenMP_SimdlenClause : OpenMP_SimdlenClauseSkip<>;
 
-//===----------------------------------------------------------------------===//
-// V5.2: [9.1.1] `sizes` clause
-//===----------------------------------------------------------------------===//
-
-class OpenMP_TileSizesClauseSkip<
-    bit traits = false, bit arguments = false, bit assemblyFormat = false,
-    bit description = false, bit extraClassDeclaration = false
-  > : OpenMP_Clause<traits, arguments, assemblyFormat, description,
-                    extraClassDeclaration> {
-  let arguments = (ins
-      OptionalAttr<DenseI64ArrayAttr>:$tile_sizes
-  );
-}
-
-def OpenMP_TileSizesClause : OpenMP_TileSizesClauseSkip<>;
-
 //===----------------------------------------------------------------------===//
 // V5.2: [5.5.9] `task_reduction` clause
 //===----------------------------------------------------------------------===//
@@ -1418,6 +1402,22 @@ class OpenMP_ThreadLimitClauseSkip<
 
 def OpenMP_ThreadLimitClause : OpenMP_ThreadLimitClauseSkip<>;
 
+//===----------------------------------------------------------------------===//
+// V5.2: [9.1.1] `sizes` clause
+//===----------------------------------------------------------------------===//
+
+class OpenMP_TileSizesClauseSkip<
+    bit traits = false, bit arguments = false, bit assemblyFormat = false,
+    bit description = false, bit extraClassDeclaration = false
+  > : OpenMP_Clause<traits, arguments, assemblyFormat, description,
+                    extraClassDeclaration> {
+  let arguments = (ins
+      OptionalAttr<DenseI64ArrayAttr>:$tile_sizes
+  );
+}
+
+def OpenMP_TileSizesClause : OpenMP_TileSizesClauseSkip<>;
+
 //===----------------------------------------------------------------------===//
 // V5.2: [12.1] `untied` clause
 //===----------------------------------------------------------------------===//

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

@@ -627,7 +627,6 @@ def LoopNestOp : OpenMP_Op<"loop_nest", traits = [
     that should be collapsed (1 if no collapse is done) after any tiling is
     performed. The tiling sizes is represented by the tile sizes clause.
 
-
     The lower and upper bounds specify a half-open range: the range includes the
     lower bound but does not include the upper bound. If the `loop_inclusive`
     attribute is specified then the upper bound is also included.
@@ -639,7 +638,7 @@ def LoopNestOp : OpenMP_Op<"loop_nest", traits = [
     `loop_steps` arguments.
 
     ```mlir
-    omp.loop_nest (%i1, %i2) : i32 = (%c0, %c0) to (%c10, %c10) step (%c1, %c1)  collapse(2) tiles(5,5) {
+    omp.loop_nest (%i1, %i2) : i32 = (%c0, %c0) to (%c10, %c10) step (%c1, %c1) collapse(2) tiles(5,5) {
       %a = load %arrA[%i1, %i2] : memref<?x?xf32>
       %b = load %arrB[%i1, %i2] : memref<?x?xf32>
       %sum = arith.addf %a, %b : f32

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

@@ -3046,11 +3046,11 @@ convertOmpLoopNest(Operation &opInst, llvm::IRBuilderBase &builder,
 
   // Do tiling.
   if (const auto &tiles = loopOp.getTileSizes()) {
-    llvm::Type *IVType = loopInfos.front()->getIndVarType();
+    llvm::Type *ivType = loopInfos.front()->getIndVarType();
     SmallVector<llvm::Value *> TileSizes;
 
     for (auto tile : tiles.value()) {
-      llvm::Value *TileVal = llvm::ConstantInt::get(IVType, tile);
+      llvm::Value *TileVal = llvm::ConstantInt::get(ivType, tile);
       TileSizes.push_back(TileVal);
     }