[llvm-branch-commits] [mlir] [MLIR][OpenMP] Add host_eval clause to omp.target (PR #116049)

[Sergio Afonso via llvm-branch-commits]

================
@@ -1166,9 +1166,10 @@ def TargetOp : OpenMP_Op<"target", traits = [
   ], clauses = [
     // TODO: Complete clause list (defaultmap, uses_allocators).
     OpenMP_AllocateClause, OpenMP_DependClause, OpenMP_DeviceClause,
-    OpenMP_HasDeviceAddrClause, OpenMP_IfClause, OpenMP_InReductionClause,
-    OpenMP_IsDevicePtrClause, OpenMP_MapClauseSkip<assemblyFormat = true>,
-    OpenMP_NowaitClause, OpenMP_PrivateClause, OpenMP_ThreadLimitClause
+    OpenMP_HasDeviceAddrClause, OpenMP_HostEvalClause, OpenMP_IfClause,
----------------
skatrak wrote:

Thank you Anchu for taking a look!

I don't think it would be a good idea to modify the dialect prefix (`OpenMP_`) here, since it's intended to represent a sort of namespace for all definitions of the dialect (https://mlir.llvm.org/docs/DefiningDialects/Operations/#class-name-and-namespaces).

The `host_eval` clause is only used for `omp.target`, so it could be integrated into that operation's definition rather than defined as its own thing. However, this would make the operation less readable and the automatically-generated operand structure for it wouldn't have information for these operands, making its constructor and Flang lowering code slightly more convoluted. Another case of this is the `OpenMP_LoopRelatedClause` definition, which is only used by a single operation and does not refer to an actual clause in the spec.

In my opinion, the advantages of having these pseudo-clauses defined as clauses outweighs the potential inconvenience of making readers have to look at their definition to realize what they represent. Perhaps a name change like "LoopRelatedClause -> LoopRelatedPseudoClause" and "HostEvalClause -> HostEvalPseudoClause" can point out this difference without changing the structure, but I'm not sure that this distinction is very useful. Let me know what you think.

https://github.com/llvm/llvm-project/pull/116049