[flang-commits] [flang] [mlir] [OpenMP][mlir] Add DynGroupPrivateClause in omp dialect (PR #153562)

[via flang-commits]

skc7 wrote:

> > > Is it semantically equivalent to having the clause placed at the `target` instead of the `teams` level? If so, you might want to consider making it a host-evaluated clause like `num_teams` or `thread_limit`. Then, the size would always be available when translating the `omp.target` operation, so you'd be able to pass it to the `createTarget` OMPIRBuilder call.
> > 
> > 
> > Thanks for the suggestion. The dyn_groupprivate clause is allowed on both target and teams directives independently per the OpenMP 6.1 spec. In the current implementation, I've placed it on both omp.target and omp.teams to match the spec.
> 
> I'm reading from the spec draft that `dyn_groupprivate` is a target-consistent clause, meaning (paraphrasing here) that its evaluation results in the same value when used on an immediately nested construct of `target` (in this case, `teams`) as when specified on that `target` construct. This is also a property of `num_teams` and `thread_limit`, so my understanding is that the size can be indeed a host-evaluated expression for our purposes here.
> 
> `teams dyn_groupprivate` has been marked as TODO, so we can work on this as a followup patch. But I do think that adding support for it would take a similar approach as what we have for `num_teams` and `thread_limit`. On the host module, we'd have something like what I show below to represent it, and it would allow us to evaluate the size expression prior to the call to `createTarget()`, which is where we can use it.
> 
> ```mlir
> %size = ... : i32
> omp.target host_eval(%size -> %arg0 : i32) ... {
>   omp.teams dyn_groupprivate(..., %arg0 : i32) ... {
>     ...
>     omp.terminator
>   }
>   omp.terminator
> }
> ```
> 
> Another unrelated comment: The `dyn_groupprivate` clause is not unique, meaning users could specify it multiple times, but the current MLIR representation and OMPIRBuilder support a single set of size + modifiers. My understanding is that we can't add them all together in the frontend because they may have different modifiers associated, so we should make changes to the clause representation and the OMPIRBuilder.
> 
> We can probably leave this for a followup patch as well, and when implementing Flang lowering make sure we trigger a not-yet-implemented error if multiple instances of the clause are found.

@skatrak  Thanks for the explanation. Both points make sense:
For `teams dyn_groupprivate`, I agree it should follow the host-eval pattern like num_teams/thread_limit. Since teams dyn_groupprivate is marked TODO, I'll handle this in a follow-up patch using the host_eval approach you described.

For multiple `dyn_groupprivate` clauses, I'll defer this to a follow-up as well. When implementing Flang lowering, we can emit a not-yet-implemented error if multiple instances are found, and then extend the clause representation and OMPIRBuilder to support it properly.

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