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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/include/llvm/Frontend/OpenMP/OMPKinds.def | ||
|---|---|---|
| 386 | I think this was commited as part of 89d9dba2c6885949887edf4b80e1aabf8d8f3f88 (with a different but compatible type). | |
| 640 | 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. | |
| 709–710 | A fork doesn't need to return, depending on the user function, and it can sync. | |
| 710 | taskwait is a barrier-like thing | |
| 764 | These are barrier-like as well. | |
| 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. | |
| 640 | 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? | |
| 709–710 | How do the function attributes apply to function pointer arguments? | |
| 710 | Noted. | |
| 763–764 | This is the other fork. | |
| 764 | Anything special about the target runtime functions? I'm wondering if all of them could fall under GetterArgWrite. | |
Comment Actions
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. | |
| 640 |
I hope not, if so it's a design bug.
I think you are not wrong. I guess it is more "natural" to say InaccessibleArgMemOnly and readonly for the ident_t though.
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. | |
| 764 | 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. | |
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 640 | I think (for now) we should even remove willreturn here. I'm not sure we have forward progress guarantees for C + OpenMP :( | |
| 674 | Here and above, nocapture. | |
| 685 | 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. | |
| 789 | 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. | |
Comment Actions
Removed usage of alignment and deference attributes and changed several functions to use barrier attributes.
Comment Actions
Just in case you haven't seen already, clang/test/OpenMP/barrier_codegen.cpp needs to be updated as well.
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
Comment Actions
They might be flaky, incompatible with your environment, or broken by something else. You should not assume master is always working ;)
I think this was commited as part of 89d9dba2c6885949887edf4b80e1aabf8d8f3f88 (with a different but compatible type).