This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add Additional Function Attribute Information to OMPKinds.def
ClosedPublic

Authored by jhuber6 on Jun 2 2020, 1:24 PM.

Details

Summary

This patch adds more function attribute information to the runtime function definitions in OMPKinds.def. The goal is to provide sufficient information about OpenMP runtime functions to perform more optimizations on OpenMP code.

Diff Detail

Event Timeline

jhuber6 created this revision.Jun 2 2020, 1:24 PM
jdoerfert added inline comments.Jun 2 2020, 2:45 PM
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
386

I think this was commited as part of 89d9dba2c6885949887edf4b80e1aabf8d8f3f88 (with a different but compatible type).

660

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.

703–704

A fork doesn't need to return, depending on the user function, and it can sync.

704

taskwait is a barrier-like thing

758

These are barrier-like as well.

jhuber6 marked 6 inline comments as done.Jun 2 2020, 3:03 PM
jhuber6 added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
386

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.

660

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?

703–704

How do the function attributes apply to function pointer arguments?

704

Noted.

757–758

This is the other fork.

758

Anything special about the target runtime functions? I'm wondering if all of them could fall under GetterArgWrite.

jhuber6 updated this revision to Diff 268241.Jun 3 2020, 9:41 AM

Small changes

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

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.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
386

Yes, and eventually we want to put the struct type in here too I guess.

660

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.

758

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.

jhuber6 updated this revision to Diff 270239.Jun 11 2020, 2:35 PM

Adding parameter and return attributes for pointers. There are some arguments I wasn't sure how they should be classified, like if every argument should be NoCapture because the lifetime of the memory isn't visible to the caller. For returned void pointers I assumed that they were aligned by eight in cases I found that the return value was a pointer to an OpenMP Runtime struct, otherwise I set it to one. I might need to double check some of the attributes to make sure they make sense.

Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2020, 2:35 PM
jdoerfert accepted this revision.Jul 16 2020, 2:42 PM

Apologies for the wait. I think with the comments below this is good to go. If you have questions or concerns, let me know. Thanks for these changes!

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
660

I think (for now) we should even remove willreturn here. I'm not sure we have forward progress guarantees for C + OpenMP :(

694

Here and above, nocapture.

705

I guess this means the attribute is returned, right? If we are sure it is, we can add the returned attribute. I checked __kmpc_threadprivate_cached and I wasn't sure the argument is actually returned, maybe I misunderstand what this means. We could add a comment here.

We should also not go for alignment or dereferenceability for now.

783

I think this will mitigate PR46210, which is good!

llvm/test/Transforms/OpenMP/add_attributes.ll
1543

the single ones should be barrier like

1552

barrier like

1615

barrier like, probably all wait functions

1654

this is a barrier like directive. Let's just mark all doacross as such for now.

This revision is now accepted and ready to land.Jul 16 2020, 2:42 PM
jhuber6 updated this revision to Diff 278905.Jul 17 2020, 2:44 PM

Removed usage of alignment and deference attributes and changed several functions to use barrier attributes.

This revision was automatically updated to reflect the committed changes.

Just in case you haven't seen already, clang/test/OpenMP/barrier_codegen.cpp needs to be updated as well.

Just in case you haven't seen already, clang/test/OpenMP/barrier_codegen.cpp needs to be updated as well.

Also chiming in to say we're seeing these failures on our bots (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8874490785658285184/+/steps/clang/0/steps/test/0/stdout?format=raw). Could you send out an update?

Just in case you haven't seen already, clang/test/OpenMP/barrier_codegen.cpp needs to be updated as well.

Also chiming in to say we're seeing these failures on our bots (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8874490785658285184/+/steps/clang/0/steps/test/0/stdout?format=raw). Could you send out an update?

Very sorry, in an earlier version I had that fixed but for whatever reason it got lost somewhere when I did a pull and I didn't notice. What's bizarre is that I ran a check-all and it didn't seem to catch it on my end. I'll push an update with that fixed and removing the unused attributes ASAP.

jhuber6 updated this revision to Diff 278949.Jul 17 2020, 5:53 PM

Fixing errors caused by unused attribute sets. Adding missing attributes to barrier_codegen.cpp.

Should I go ahead and commit this considering the previous was temporarily reverted? Or should I just wait a bit to see if it fails testing again.

Fixing errors caused by unused attribute sets. Adding missing attributes to barrier_codegen.cpp.

Should I go ahead and commit this considering the previous was temporarily reverted? Or should I just wait a bit to see if it fails testing again.

If you only did minor modifications and no major problem showed up, the previous LGTM still stands. You should (always) run a full make check-all locally (or better on a server) to verify no other issues are known.

FWIW, it happens that we break bots and patches get reverted. That is not great but also not too bad.

Fixing errors caused by unused attribute sets. Adding missing attributes to barrier_codegen.cpp.

Should I go ahead and commit this considering the previous was temporarily reverted? Or should I just wait a bit to see if it fails testing again.

If you only did minor modifications and no major problem showed up, the previous LGTM still stands. You should (always) run a full make check-all locally (or better on a server) to verify no other issues are known.

FWIW, it happens that we break bots and patches get reverted. That is not great but also not too bad.

I did run a check-all but apparently the last build I did didn't build clang for some reason so it wasn't included in the tests. I ran cmake again with the same options and it seemed to build clang this time so I don't know what that was about.

After that I ran the tests again and I passed everything except these four. I reverted my commit and pulled from master and still failed the same test so I'm assuming it's a problem with my CUDA installation rather than the patch.

libomp :: env/kmp_set_dispatch_buf.c
libomp :: worksharing/for/kmp_set_dispatch_buf.c
libomptarget :: mapping/declare_mapper_target_update.cpp
libomptarget :: offloading/target_depend_nowait.cpp

Fixing errors caused by unused attribute sets. Adding missing attributes to barrier_codegen.cpp.

Should I go ahead and commit this considering the previous was temporarily reverted? Or should I just wait a bit to see if it fails testing again.

If you only did minor modifications and no major problem showed up, the previous LGTM still stands. You should (always) run a full make check-all locally (or better on a server) to verify no other issues are known.

FWIW, it happens that we break bots and patches get reverted. That is not great but also not too bad.

I did run a check-all but apparently the last build I did didn't build clang for some reason so it wasn't included in the tests. I ran cmake again with the same options and it seemed to build clang this time so I don't know what that was about.

After that I ran the tests again and I passed everything except these four. I reverted my commit and pulled from master and still failed the same test so I'm assuming it's a problem with my CUDA installation rather than the patch.

libomp :: env/kmp_set_dispatch_buf.c
libomp :: worksharing/for/kmp_set_dispatch_buf.c
libomptarget :: mapping/declare_mapper_target_update.cpp
libomptarget :: offloading/target_depend_nowait.cpp

They might be flaky, incompatible with your environment, or broken by something else. You should not assume master is always working ;)