[Mlir-commits] [mlir] [MLIR][OpenMP] Add `OpenMP_Clause` tablegen definitions (PR #92521)

[Sergio Afonso]

skatrak wrote:

Thank you @bhandarkar-pranav for taking a look! Since both of your comments are related, I'll try to explain the reasoning here. Hopefully it makes sense to you.

Basically the idea is that we want to have `def`s for each clause that do not skip anything, which will be the main ones used while defining operations, but we also want the ability to skip parts of them in a very small amount of edge cases. That's what the separation is for, which I think we are both onboard with.

Then there is the definition of the `OpenMP_XyzClauseSkip` classes, which is the one that seems to be causing the confusion. How these are supposed to be read is "An Xyz clause skipping the following fields: ...". So, when we do `def OpenMP_XyzClause : OpenMP_XyzClauseSkip<>` we're saying: "Define an Xyz clause skipping none of the fields". I think this is more readable especially when actually skipping fields while defining operations. For example:
```
def TaskloopOp : OpenMP_Op<"taskloop", traits = [ ... ],
    clauses = [ OpenMP_ReductionClauseSkip<extraClassDeclaration = true, ...> ]> { ... }
```
In that case, we're saying that the `omp.taskloop` operation takes a reduction clause but we want to skip the `extraClassDeclaration` field (and possibly others) inherited from it.

I think keeping the "skip" prefix on the template parameters would make things redundant (e.g. `OpenMP_XyzClauseSkip<skipDescription = true>`). Alternatively, we could make the base class use the regular clause name: `OpenMP_XyzClause<skipDescription = true>`, which was my first idea. The disadvantage of this is that then we'd have two options that in my opinion would be worse because they make the much more common case more verbose:
- Renaming the definition that includes every field into something like `OpenMP_XyzClause{Base,Default,NoSkip}`.
- Instantiating the template directly in the clause list every time: `traits = [ OpenMP_Xyz1Clause<>, OpenMP_Xyz2Clause<skipDescription = true> ]`

In the end, I thought that `traits = [ OpenMP_Xyz1Clause, OpenMP_Xyz2ClauseSkip<description = true> ]` was the most readable alternative of the options I considered.

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