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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 6 inline comments as done.
jhuber6 added inline comments.


================
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)
 
----------------
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.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:594
+                                   EnumAttr(WillReturn)) 
                     : AttributeSet(EnumAttr(NoUnwind)))
 
----------------
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?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:611
 __OMP_RTL_ATTRS(__kmpc_global_thread_num, GetterAttrs, AttributeSet(), {})
-__OMP_RTL_ATTRS(__kmpc_fork_call, AttributeSet(EnumAttr(NoUnwind)),
-                AttributeSet(), {})
-__OMP_RTL_ATTRS(__kmpc_omp_taskwait, AttributeSet(), AttributeSet(), {})
-__OMP_RTL_ATTRS(__kmpc_omp_taskyield,
-                AttributeSet(EnumAttr(InaccessibleMemOrArgMemOnly)),
-                AttributeSet(), {})
-__OMP_RTL_ATTRS(__kmpc_push_num_threads,
-                AttributeSet(EnumAttr(InaccessibleMemOrArgMemOnly)),
-                AttributeSet(), {})
-__OMP_RTL_ATTRS(__kmpc_push_proc_bind,
-                AttributeSet(EnumAttr(InaccessibleMemOrArgMemOnly)),
+__OMP_RTL_ATTRS(__kmpc_fork_call, DefaultAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__kmpc_omp_taskwait, ArgOnlyAttrs, AttributeSet(), {})
----------------
jdoerfert wrote:
> A fork doesn't need to return, depending on the user function, and it can sync. 
How do the function attributes apply to function pointer arguments?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:612
+__OMP_RTL_ATTRS(__kmpc_fork_call, DefaultAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__kmpc_omp_taskwait, ArgOnlyAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__kmpc_omp_taskyield, ArgOnlyAttrs, AttributeSet(), {})
----------------
jdoerfert wrote:
> taskwait is a barrier-like thing
Noted.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:763
 
-__OMP_RTL_ATTRS(__kmpc_fork_teams, AttributeSet(EnumAttr(NoUnwind)),
-                AttributeSet(), {})
-__OMP_RTL_ATTRS(__kmpc_push_num_teams, AttributeSet(EnumAttr(NoUnwind)),
-                AttributeSet(), {})
+__OMP_RTL_ATTRS(__kmpc_fork_teams, DefaultAttrs, AttributeSet(), {})
+__OMP_RTL_ATTRS(__kmpc_push_num_teams, ArgOnlyAttrs, AttributeSet(), {})
----------------
This is the other fork.


================
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(), {})
----------------
Anything special about the target runtime functions? I'm wondering if all of them could fall under GetterArgWrite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81031