Address review comments

Author: skatrak

mlir/include/mlir/Dialect/OpenMP/OpenMPClauseOperands.h

@@ -53,6 +53,10 @@ struct CopyprivateClauseOps {
 };
 
 struct CriticalNameClauseOps {
+  /// This field has a generic name because it's mirroring the `sym_name`
+  /// argument of the `OpenMP_CriticalNameClause` tablegen definition. That one
+  /// can't be renamed to anything more specific because the `sym_name` name is
+  /// a requirement of the `Symbol` MLIR trait associated with that clause.
   StringAttr symName;
 };
 

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

@@ -20,6 +20,7 @@
 #define OPENMP_CLAUSES
 
 include "mlir/Dialect/OpenMP/OpenMPOpBase.td"
+include "mlir/IR/SymbolInterfaces.td"
 
 //===----------------------------------------------------------------------===//
 // V5.2: [5.11] `aligned` clause
@@ -175,6 +176,10 @@ class OpenMP_CriticalNameClauseSkip<
     bit description = false, bit extraClassDeclaration = false
   > : OpenMP_Clause</*isRequired=*/true, traits, arguments, assemblyFormat,
                     description, extraClassDeclaration> {
+  let traits = [
+    Symbol
+  ];
+
   let arguments = (ins
     SymbolNameAttr:$sym_name
   );
@@ -455,11 +460,11 @@ class OpenMP_IfClauseSkip<
   > : OpenMP_Clause</*isRequired=*/false, traits, arguments, assemblyFormat,
                     description, extraClassDeclaration> {
   let arguments = (ins
-    Optional<I1>:$if_var
+    Optional<I1>:$if_expr
   );
 
   let assemblyFormat = [{
-    `if` `(` $if_var `)`
+    `if` `(` $if_expr `)`
   }];
 
   // Description varies depending on the operation.

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

@@ -145,7 +145,7 @@ def ParallelOp : OpenMP_Op<"parallel", traits = [
     The parallel construct includes a region of code which is to be executed
     by a team of threads.
 
-    The optional `if_var` parameter specifies a boolean result of a conditional
+    The optional `if_expr` parameter specifies a boolean result of a conditional
     check. If this value is 1 or is not provided then the parallel region runs
     as normal, if it is 0 then the parallel region is executed with one thread.
   }] # clausesDescription;
@@ -161,7 +161,7 @@ def ParallelOp : OpenMP_Op<"parallel", traits = [
   // no longer skipped for clauses added to this operation at that time.
   let assemblyFormat = [{
     oilist(
-          `if` `(` $if_var `)`
+          `if` `(` $if_expr `)`
           | `num_threads` `(` $num_threads `:` type($num_threads) `)`
           | `allocate` `(`
               custom<AllocateAndAllocator>(
@@ -206,7 +206,7 @@ def TeamsOp : OpenMP_Op<"teams", traits = [
     league of teams. Once created, the number of teams remains constant for the
     duration of its code region.
 
-    If the `if_var` is present and it evaluates to `false`, the number of teams
+    If the `if_expr` is present and it evaluates to `false`, the number of teams
     created is one.
   }] # clausesDescription;
 
@@ -568,7 +568,7 @@ def TaskOp : OpenMP_Op<"task", traits = [
     "mergeable task", please check OpenMP Specification.
 
     When an `if` clause is present on a task construct, and the value of
-    `if_var` evaluates to `false`, an "undeferred task" is generated, and the
+    `if_expr` evaluates to `false`, an "undeferred task" is generated, and the
     encountering thread must suspend the current task region, for which
     execution cannot be resumed until execution of the structured block that is
     associated with the generated task is completed.
@@ -964,7 +964,7 @@ def TargetDataOp: OpenMP_Op<"target_data", traits = [
     to and from the offloading device when multiple target regions are using
     the same data.
 
-    The optional `if_var` parameter specifies a boolean result of a conditional
+    The optional `if_expr` parameter specifies a boolean result of a conditional
     check. If this value is 1 or is not provided then the target region runs on
     a device, if it is 0 then the target region is executed on the host device.
   }] # clausesDescription;
@@ -993,7 +993,7 @@ def TargetEnterDataOp: OpenMP_Op<"target_enter_data", traits = [
     a device data environment. The target enter data directive is a
     stand-alone directive.
 
-    The optional `if_var` parameter specifies a boolean result of a conditional
+    The optional `if_expr` parameter specifies a boolean result of a conditional
     check. If this value is 1 or is not provided then the target region runs on
     a device, if it is 0 then the target region is executed on the host device.
   }] # clausesDescription;
@@ -1022,7 +1022,7 @@ def TargetExitDataOp: OpenMP_Op<"target_exit_data", traits = [
     device data environment. The target exit data directive is
     a stand-alone directive.
 
-    The optional `if_var` parameter specifies a boolean result of a conditional
+    The optional `if_expr` parameter specifies a boolean result of a conditional
     check. If this value is 1 or is not provided then the target region runs on
     a device, if it is 0 then the target region is executed on the host device.
   }] # clausesDescription;
@@ -1052,7 +1052,7 @@ def TargetUpdateOp: OpenMP_Op<"target_update", traits = [
     specified motion clauses. The target update construct is a stand-alone
     directive.
 
-    The optional `if_var` parameter specifies a boolean result of a conditional
+    The optional `if_expr` parameter specifies a boolean result of a conditional
     check. If this value is 1 or is not provided then the target region runs on
     a device, if it is 0 then the target region is executed on the host device.
 
@@ -1090,7 +1090,7 @@ def TargetOp : OpenMP_Op<"target", traits = [
     The target construct includes a region of code which is to be executed
     on a device.
 
-    The optional `if_var` parameter specifies a boolean result of a conditional
+    The optional `if_expr` parameter specifies a boolean result of a conditional
     check. If this value is 1 or is not provided then the target region runs on
     a device, if it is 0 then the target region is executed on the host device.
   }] # clausesDescription;
@@ -1119,9 +1119,7 @@ def MasterOp : OpenMP_Op<"master", singleRegion = true> {
 //===----------------------------------------------------------------------===//
 // 2.17.1 critical Construct
 //===----------------------------------------------------------------------===//
-def CriticalDeclareOp : OpenMP_Op<"critical.declare", traits = [
-    Symbol
-  ], clauses = [
+def CriticalDeclareOp : OpenMP_Op<"critical.declare", clauses = [
     OpenMP_CriticalNameClause, OpenMP_HintClause
   ]> {
   let summary = "declares a named critical section.";

mlir/lib/Conversion/SCFToOpenMP/SCFToOpenMP.cpp

@@ -444,7 +444,7 @@ struct ParallelOpLowering : public OpRewritePattern<scf::ParallelOp> {
     // Create the parallel wrapper.
     auto ompParallel = rewriter.create<omp::ParallelOp>(
         loc,
-        /* if_var = */ Value{},
+        /* if_expr = */ Value{},
         /* num_threads = */ numThreadsVar,
         /* allocate_vars = */ llvm::SmallVector<Value>{},
         /* allocator_vars = */ llvm::SmallVector<Value>{},

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

@@ -1474,7 +1474,7 @@ LogicalResult TargetOp::verify() {
 void ParallelOp::build(OpBuilder &builder, OperationState &state,
                        ArrayRef<NamedAttribute> attributes) {
   ParallelOp::build(
-      builder, state, /*if_var=*/nullptr, /*num_threads=*/nullptr,
+      builder, state, /*if_expr=*/nullptr, /*num_threads=*/nullptr,
       /*allocate_vars=*/ValueRange(), /*allocator_vars=*/ValueRange(),
       /*reduction_vars=*/ValueRange(), /*reduction_byref=*/nullptr,
       /*reduction_syms=*/nullptr, /*proc_bind_kind=*/nullptr,

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

@@ -1000,7 +1000,7 @@ convertOmpTeams(omp::TeamsOp op, llvm::IRBuilderBase &builder,
     threadLimit = moduleTranslation.lookupValue(threadLimitVar);
 
   llvm::Value *ifExpr = nullptr;
-  if (Value ifVar = op.getIfVar())
+  if (Value ifVar = op.getIfExpr())
     ifExpr = moduleTranslation.lookupValue(ifVar);
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
@@ -1061,7 +1061,7 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder,
   builder.restoreIP(moduleTranslation.getOpenMPBuilder()->createTask(
       ompLoc, allocaIP, bodyCB, !taskOp.getUntied(),
       moduleTranslation.lookupValue(taskOp.getFinal()),
-      moduleTranslation.lookupValue(taskOp.getIfVar()), dds));
+      moduleTranslation.lookupValue(taskOp.getIfExpr()), dds));
   return bodyGenStatus;
 }
 
@@ -1559,7 +1559,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
   };
 
   llvm::Value *ifCond = nullptr;
-  if (auto ifVar = opInst.getIfVar())
+  if (auto ifVar = opInst.getIfExpr())
     ifCond = moduleTranslation.lookupValue(ifVar);
   llvm::Value *numThreads = nullptr;
   if (auto numThreadsVar = opInst.getNumThreads())
@@ -1678,8 +1678,8 @@ convertOmpSimd(Operation &opInst, llvm::IRBuilderBase &builder,
   llvm::MapVector<llvm::Value *, llvm::Value *> alignedVars;
   llvm::omp::OrderKind order = convertOrderKind(simdOp.getOrder());
   ompBuilder->applySimd(loopInfo, alignedVars,
-                        simdOp.getIfVar()
-                            ? moduleTranslation.lookupValue(simdOp.getIfVar())
+                        simdOp.getIfExpr()
+                            ? moduleTranslation.lookupValue(simdOp.getIfExpr())
                             : nullptr,
                         order, simdlen, safelen);
 
@@ -2790,7 +2790,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
   LogicalResult result =
       llvm::TypeSwitch<Operation *, LogicalResult>(op)
           .Case([&](omp::TargetDataOp dataOp) {
-            if (auto ifVar = dataOp.getIfVar())
+            if (auto ifVar = dataOp.getIfExpr())
               ifCond = moduleTranslation.lookupValue(ifVar);
 
             if (auto devId = dataOp.getDevice())
@@ -2809,7 +2809,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
               return (LogicalResult)(enterDataOp.emitError(
                   "`nowait` is not supported yet"));
 
-            if (auto ifVar = enterDataOp.getIfVar())
+            if (auto ifVar = enterDataOp.getIfExpr())
               ifCond = moduleTranslation.lookupValue(ifVar);
 
             if (auto devId = enterDataOp.getDevice())
@@ -2826,7 +2826,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
               return (LogicalResult)(exitDataOp.emitError(
                   "`nowait` is not supported yet"));
 
-            if (auto ifVar = exitDataOp.getIfVar())
+            if (auto ifVar = exitDataOp.getIfExpr())
               ifCond = moduleTranslation.lookupValue(ifVar);
 
             if (auto devId = exitDataOp.getDevice())
@@ -2844,7 +2844,7 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
               return (LogicalResult)(updateDataOp.emitError(
                   "`nowait` is not supported yet"));
 
-            if (auto ifVar = updateDataOp.getIfVar())
+            if (auto ifVar = updateDataOp.getIfExpr())
               ifCond = moduleTranslation.lookupValue(ifVar);
 
             if (auto devId = updateDataOp.getDevice())
@@ -3012,7 +3012,7 @@ static bool getTargetEntryUniqueInfo(llvm::TargetRegionEntryInfo &targetInfo,
 
 static bool targetOpSupported(Operation &opInst) {
   auto targetOp = cast<omp::TargetOp>(opInst);
-  if (targetOp.getIfVar()) {
+  if (targetOp.getIfExpr()) {
     opInst.emitError("If clause not yet supported");
     return false;
   }

mlir/test/Dialect/OpenMP/ops.mlir

@@ -804,7 +804,7 @@ func.func @omp_distribute(%chunk_size : i32, %data_var : memref<i32>, %arg0 : i3
 // CHECK-LABEL: omp_target
 func.func @omp_target(%if_cond : i1, %device : si32,  %num_threads : i32, %device_ptr: memref<i32>, %device_addr: memref<?xi32>, %map1: memref<?xi32>, %map2: memref<?xi32>) -> () {
 
-    // Test with optional operands; if_var, device, thread_limit, private, firstprivate and nowait.
+    // Test with optional operands; if_expr, device, thread_limit, private, firstprivate and nowait.
     // CHECK: omp.target if({{.*}}) device({{.*}}) thread_limit({{.*}}) nowait
     "omp.target"(%if_cond, %device, %num_threads) ({
        // CHECK: omp.terminator
@@ -2165,7 +2165,7 @@ func.func @omp_threadprivate() {
 llvm.mlir.global internal @_QFsubEx() : i32
 
 func.func @omp_cancel_parallel(%if_cond : i1) -> () {
-  // Test with optional operand; if_var.
+  // Test with optional operand; if_expr.
   omp.parallel {
     // CHECK: omp.cancel cancellation_construct_type(parallel) if(%{{.*}})
     omp.cancel cancellation_construct_type(parallel) if(%if_cond)

mlir/test/Target/LLVMIR/openmp-llvm.mlir

@@ -148,12 +148,12 @@ llvm.func @test_omp_parallel_num_threads_3() -> () {
 // CHECK: define internal void @[[OMP_OUTLINED_FN_NUM_THREADS_3_1]]
   // CHECK: call void @__kmpc_barrier
 
-// CHECK: define void @test_omp_parallel_if_1(i32 %[[IF_VAR_1:.*]])
+// CHECK: define void @test_omp_parallel_if_1(i32 %[[IF_EXPR_1:.*]])
 llvm.func @test_omp_parallel_if_1(%arg0: i32) -> () {
 
   %0 = llvm.mlir.constant(0 : index) : i32
   %1 = llvm.icmp "slt" %arg0, %0 : i32
-// CHECK: %[[IF_COND_VAR_1:.*]] = icmp slt i32 %[[IF_VAR_1]], 0
+// CHECK: %[[IF_COND_VAR_1:.*]] = icmp slt i32 %[[IF_EXPR_1]], 0
 
 
 // CHECK: %[[GTN_IF_1:.*]] = call i32 @__kmpc_global_thread_num(ptr @[[SI_VAR_IF_1:.*]])