Page MenuHomePhabricator

Adding Extra Runtime Functions to OMPKinds.def
AcceptedPublic

Authored by jhuber6 on May 11 2020, 12:50 PM.

Details

Reviewers
jdoerfert
Summary

Beginning to add missing runtime functions to OMPKinds.def. I haven't added any of the target-offloading runtime functions yet. I've added the definitions to the attributes test file but have not added any attributes to OMPKinds.def.

Diff Detail

Event Timeline

jhuber6 created this revision.May 11 2020, 12:50 PM

Thx! Can u make all of them at least nounwind? These are "only" ones clang currently generates, right?

jhuber6 added a comment.EditedMay 11 2020, 1:14 PM

I got most of them by getting the LLVM IR for the test files in openmp/runtime/test and took the unique calls, then filled in the gaps for some variants and others I knew of. There may be a few that Clang doesn't generate, I think kmpc_begin() is always implicit. I wasn't sure if I could declare them all as nounwind since there were some other kmpc* calls in the add_attributes.ll that didn't show any attributes so I was playing on the safe side. I could add them all as nounwind if it's safe.

Also I should mention that I just treated the task_t types as void pointers. I could find their definition in the LLVM IR but they were complicated and I felt like it wasn't worth the effort for now.

I got most of them by getting the LLVM IR for the test files in openmp/runtime/test and took the unique calls, then filled in the gaps for some variants and others I knew of. There may be a few that Clang doesn't generate, I think kmpc_begin() is always implicit.

Good. We don't necessarily need internal ones but all that are generated help us.

I wasn't sure if I could declare them all as nounwind since there were some other kmpc* calls in the add_attributes.ll that didn't show any attributes so I was playing on the safe side. I could add them all as nounwind if it's safe.

nounwind is safe. Where it is missing we just haven't added it in the .def file.

Also I should mention that I just treated the task_t types as void pointers. I could find their definition in the LLVM IR but they were complicated and I felt like it wasn't worth the effort for now.

That should be fine for now.

jhuber6 updated this revision to Diff 263336.May 11 2020, 7:03 PM

Adding nounwind attributes.

jhuber6 marked an inline comment as done.May 11 2020, 7:04 PM
jhuber6 added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
479

This should be a void* and its corresponding definition an i8* but since the types come from the getXXXTy methods I wasn't sure how to get an i8***.

jdoerfert added inline comments.May 11 2020, 8:37 PM
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
479

No way yet, but I think this should work just fine: https://reviews.llvm.org/D79675#inline-730836

Should I start adding the target offload runtime functions?

jhuber6 marked an inline comment as done.May 12 2020, 7:10 AM
jhuber6 added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
444

Functions like this one use "size_t" which would be a 32 bit integer on a 32 bit system, but I've just set it to 64 bits for now. Should I add some macro's to set it to an Int32 or Int64 depending on the target? The tests target a 64 bit system and I can't see many people using OpenMP on a 32 bit computer so I wasn't sure if I should bother.

Should I start adding the target offload runtime functions?

You can, maybe open a second review based on this one.

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

@fghanim has a solution for int and size_t in D79675. We can merge that part separately as I noted in that review. We should utilize those types in this patch right away.

So should I just add his SizeTy wherever the function uses size_t? I could also do a define of VoidPtr and Int8Ptr to make it more obvious.

jhuber6 updated this revision to Diff 263489.May 12 2020, 12:18 PM

Adding missing runtimes and a void pointer type. Replaced uses of size_t with SizeTy instead of Int64, SizeTy isn't defined yet so it won't build.

OK, now we have cyclic dependences ;)

I would suggest you split the declarations with size_t type out and we commit this patch. Once @fghanim landed the other one we commit the size_t ones.

jhuber6 updated this revision to Diff 263518.May 12 2020, 2:34 PM

I just added a temporary type that sets it to Int64 so it will compile, then it can be removed once there's an external type. Does that work?

jhuber6 marked an inline comment as done.May 12 2020, 3:06 PM
jhuber6 added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
303

Forgot to change these to VoidPtr.

jdoerfert accepted this revision.May 12 2020, 3:29 PM

LGTM after you fixed the comment you made

This revision is now accepted and ready to land.May 12 2020, 3:29 PM

@jdoerfert At this point should I just drop the changes I made in OMPKinds.def ? :D

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

@jdoerfert Should I drop these def. from my patch?

302

@jdoerfert Also, these 3?

482

@jdoerfert and these 2?

491

@jdoerfert Also, these 2?

jhuber6 updated this revision to Diff 263738.May 13 2020, 8:32 AM

Added missing VoitPtr.

@jdoerfert At this point should I just drop the changes I made in OMPKinds.def ? :D

The target dependent size stuff is not solved in here so your solution is needed. The runtime functions and types now declared do not need to be declared again by you.
I'll merge this today.

@jdoerfert At this point should I just drop the changes I made in OMPKinds.def ? :D

The target dependent size stuff is not solved in here so your solution is needed. The runtime functions and types now declared do not need to be declared again by you.
I'll merge this today.

Ok. But this patch doesn't handle or initialize any of the voidptr types in OMPConstant.cpp/h which I need for my stuff. So wouldn't it be better, to just remove the things I already defined in my patch from here, so it would work for now, and then I'll come back and clean things up when everything is merged? what do you think?

@jdoerfert At this point should I just drop the changes I made in OMPKinds.def ? :D

The target dependent size stuff is not solved in here so your solution is needed. The runtime functions and types now declared do not need to be declared again by you.
I'll merge this today.

Ok. But this patch doesn't handle or initialize any of the voidptr types in OMPConstant.cpp/h which I need for my stuff. So wouldn't it be better, to just remove the things I already defined in my patch from here, so it would work for now, and then I'll come back and clean things up when everything is merged? what do you think?

You mean because of the Fortran/Flang void* uncertainty? I mean, this should initialize VoidPtr and VoidPtrPtr just fine, right?