[PATCH] D81031: [OpenMP] Add Additional Function Attribute Information to OMPKinds.def

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D81031#2071701 <https://reviews.llvm.org/D81031#2071701>, @jhuber6 wrote:

> What should we be doing about the argument and return attributes?


After the function attributes we have return attributes and then an array of argument attributes (corresponding to the argument at that position).
Where applicable we probably want  `nofree`, `readonly`, and `writeonly` for argument pointers and for returned pointers we want `noalias`, `align`, and `dereferenceable(_or_null)`.
It's not clear that others might not be useful but these should cover most interesting things.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:340
+__OMP_RTL(__kmpc_omp_reg_task_with_affinity, false, Int32, IdentPtr, Int32,
+          VoidPtr, Int32, VoidPtr)
 
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > I think this was commited as part of 89d9dba2c6885949887edf4b80e1aabf8d8f3f88 (with a different but compatible type).
> It used to be an Int8Ptr, but I think it's more correct as a VoidPtr. On that note I should add inline comments for its actual named struct they point to so we have that information for the future.
Yes, and eventually we want to put the struct type in here too I guess.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:594
+                                   EnumAttr(WillReturn)) 
                     : AttributeSet(EnumAttr(NoUnwind)))
 
----------------
jhuber6 wrote:
> jdoerfert wrote:
> > I guess if we have `InaccessibleMemOnly` we should call the set `InaccessibleOnlyAttrs`. Though we need to also have one with `InaccessibleMemOrArgMemOnly` for functions have the `ident_t *` argument, e.g., most `__kmpc` functions that are now ArgOnlyAttrs. I think we can also add nofree to almost all of them.
> I'll change the name, I couldn't think of a good one at the time. Are the `ident_t *` values written anywhere? I figured you could treat them as constants so `InaccessibleMemOnly` would suffice, but it is probably more correct to say they're all `InaccessibleArgMemOnly`. Do you think `__kmpc_alloc` and the like are the only runtime functions that will call free?
> Are the ident_t * values written anywhere

I hope not, if so it's a design bug.

> I figured you could treat them as constants so InaccessibleMemOnly would suffice, but it is probably more correct to say they're all InaccessibleArgMemOnly

I think you are not wrong. I guess it is more "natural" to say `InaccessibleArgMemOnly` and `readonly` for the `ident_t` though.

> Do you think __kmpc_alloc and the like are the only runtime functions that will call free?

So, runtime functions might allocate and free memory, the question is if we can observe that. I think only explicit "_free" functions are interesting in this regard. Those that take a pointer from user land and free it. Everything else should probably be `nofree`.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:782
+__OMP_RTL_ATTRS(__kmpc_push_target_tripcount, ArgOnlyAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__tgt_target, DefaultAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__tgt_target_nowait, DefaultAttrs, AttributeSet(), {})
----------------
jhuber6 wrote:
> Anything special about the target runtime functions? I'm wondering if all of them could fall under GetterArgWrite.
The `*_target*` and `*_teams*` ones are basically fork functions (I guess we can make that a category too as it should include >6 with the task function).

The data functions may read and write argument data, depending on the inputs, so we can't give them readonly/writeonly here. It is also questionable if we can say InaccessibleAndArgMemOnly as it is not *100%* clear if that means argument pointee memory only or any memory reachable from the argument. I think its the former which should preclude it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81031