[Openmp-commits] [PATCH] D106509: [OpenMP][OpenACC] Implement `hold` map type modifier extension in Clang (1/2)

Thu Jul 22 09:50:53 PDT 2021

jdenny added a comment.

Thanks for the reviews.

In D106509#2896351 <https://reviews.llvm.org/D106509#2896351>, @ABataev wrote:

> That's strange that we need to ignore `delete` modifier,

It's not ignored in general: the standard (dynamic) OpenMP reference count is set to 0 (if it isn't already) so that, once `hold` is not in effect, the data can be unmapped.

> I would say that most probably there is a bug in the user's code.

Yes, my understanding of the motivation for this OpenACC feature is to protect users from unbalanced increments and decrements.  In structured cases, the assumption is that it's unlikely the user intends to unmap early.

>> Clang currently doesn't support OpenMP 5.1 features unless -fopenmp-version=51. Does it make sense to have an option to enable extensions? Instead of a separate option, we could accept something like -fopenmp-version=51,hold,foo.
>
> I would just add a new options `-fopenmp-extension` or something like this just toenable all non-standard extensions, better to keep `-fopenmp-version` as is.

Works for me.  I suppose if we later want a syntax for enabling specific extensions, `-fopenmp-extension` could become an alias for `-fopenmp-extension=all`.

>> In Clang diagnostics, this patch does not add hold to the list of acceptable map type modifiers because it's an extension. Should it? If there were a command-line option to enable extensions, that would make the path clearer.
>
> Yes, with the extensions enabled it should generate the list of all supported modifiers.

Agreed.

In D106509#2896399 <https://reviews.llvm.org/D106509#2896399>, @ABataev wrote:

> In D106509#2896398 <https://reviews.llvm.org/D106509#2896398>, @jdoerfert wrote:
>
>> I would propose we prefix these new clauses and such with `ompx_`.
>
> +1 here

That's fine for me, but don't the routines use `llvm_omp_`?

Should we also have that prefix in various enumerators in the implementation?  For example, what does `OMP_MAP_HOLD` become?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106509/new/

https://reviews.llvm.org/D106509