[RFC] Uniformize OpenMP dialect operation names

MLIR
1

I have noticed there are currently a few discrepancies on how operations in the OpenMP dialect are named, so this RFC is mainly to seek agreement on a uniform way to do this. There’s an open PR with an initial proposal, but after receiving some comments my current proposal would instead be the following:

  1. Names of C++ operation classes should follow the spelling of the corresponding directive or clause. The first letter of each word would be in uppercase. Directives like taskloop would be expressed as TaskloopOp, whereas target data would become TargetDataOp.
  2. Names used in the textual MLIR representation would match the corresponding directive or clause. In this case, spaces would be replaced by underscores, such that taskloop becomes omp.taskloop and target data becomes omp.target_data.
  3. When a single directive makes more sense to be split into multiple operations, its textual MLIR representation uses a dot to split the directive from the variant. This is the case for atomic, which is split into omp.atomic.read, omp.atomic.write, omp.atomic.update and omp.atomic.capture.
  4. Other operations that don’t directly refer to OpenMP directives or clauses should use underscores to separate each word in the textual MLIR representation. If they are expected to only ever apply to one directive or clause (i.e. they are always associated to one other operation), prefix them with its name followed by a dot.

The result of applying these rules would be the following changes:

  • omp::TaskLoopOp → omp::TaskloopOp: Point 1.
  • omp::TaskGroupOp → omp::TaskgroupOp: Point 1.
  • omp::DataBoundsOp → omp::MapBoundsOp: Point 4.
  • omp::DataOp → omp::TargetDataOp: Point 1.
  • omp::EnterDataOp → omp::TargetEnterDataOp: Point 1.
  • omp::ExitDataOp → omp::TargetExitDataOp: Point 1.
  • omp::UpdateDataOp → omp::TargetUpdateOp: Point 1.
  • omp::ReductionDeclareOp → omp::DeclareReductionOp: Point 1.
  • omp::WsLoopOp → omp::WsloopOp: For consistency, due to the lack of underscore in omp.wsloop.
  • omp.bounds → omp.map.bounds: Point 4.
  • omp.map_info → omp.map.info: Point 4.
  • omp.target_update_data → omp.target_update: Point 2.
  • omp.cancellationpoint → omp.cancellation_point: Point 2.
  • omp.ordered_region → omp.ordered.region: Point 3.
  • omp.reduction.declare → omp.declare_reduction: Point 2.

The omp::SimdLoopOp / omp.simdloop operation would exceptionally remain unchanged, but as part of the ongoing transition to using wrapper operations to represent composite constructs this operation would eventually become omp::SimdOp / omp.simd.

1 Like
2

Thank you for taking time for this. The principles in the current proposal look good to me.

3

This looks good to me.

One comment on point 3: the “atomic” word is the name of the atomic construct. The “read”, “write”, etc. are clauses. From this point of view the naming is justified by the spec, and we don’t have to only rely on it making sense. :slight_smile:

Similarly, for “ordered”—it’s a standalone construct, so anything appended to it would be separated by a period.

4

Thank you for your comments. You’re right @kparzysz, I probably didn’t convey exactly what I intended in rule 3. The idea was what you’re saying: If for whatever reason multiple ops are created for the same construct (e.g. splitting by clause like for atomic or splitting between standalone vs block-associated ordered), make the name construct_name.variant.

5

I think this is a good idea!

6

The linked PR is now updated with the approach proposed in the RFC.