This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP][MLIR] Add declare target attribute set and interface for the OpenMP dialect
ClosedPublic

Authored by agozillon on May 10 2023, 4:19 PM.

Details

Summary

This attribute represents the OpenMP declare target directive, it marks a function
or global as declare target by being present but also contains information on
the device_type and capture clause (link or to). It being an attribute allows it to
mark existing constructs and be converted trivially on lowering from the OpenMP
dialect to MLIR using amendOperation.

An interface has been made for the declare target attribute, with several helper
methods for managing the attribute, this interface can be applied to MLIR
operations that are allowed to be marked as declare target (as an example, it
is by default applied to func.func, LLVMFunc, fir.GlobalOps and LLVMGlobalOps).

Diff Detail

Event Timeline

agozillon created this revision.May 10 2023, 4:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.May 10 2023, 4:19 PM

This is the attribute and interface segment, split off from the larger declare target patch here: https://reviews.llvm.org/D146063 This segment has underwent no additions or changes during the split.

I believe this patch can go in without the semantic analysis pass patch, as can the lowering that utilises the attribute, so if it would be possible to get a review on this it would be appreciated.

Thanks @agozillon for splitting up the patch.

Should specifying a function as a declare target have any impact on whether the function can be inlined or specialized?

Thanks @agozillon for splitting up the patch.

Should specifying a function as a declare target have any impact on whether the function can be inlined or specialized?

No problem at all, I'm sorry I let the original grow so large.

Unfortunately we're not really at the stage of testing this sort of interaction (and it's not something I've invested a lot of thought into just yet), however, I think inlining should be fine to do to declare target functions, as the call locations should be occurring in functions or target regions that are being lowered to the appropriate device, so the inlined function should be getting embedded into the appropriate device function. Using the current declare target function interface it might be possible to inhibit inlining from the inliner pass if it does prove to be a problem though, although, I've not tried something like this yet, so it is speculation with my limited MLIR knowledge. I think function specialisation would also fall under a similar umbrella.

Although, someone more knowledgeable may be able to state otherwise (as I can only claim a very basic understanding of these passes), so if anyone can foresee any problems please do bring them up!

Thanks @agozillon for splitting up the patch.

I think inlining should be fine to do to declare target functions, as the call locations should be occurring in functions or target regions that are being lowered to the appropriate device, so the inlined function should be getting embedded into the appropriate device function.

OK

Would you know whether the link clause will be handled in the OpenMPIRBuilder or would it require the frontend to move the link clause info to target map/target data?

flang/include/flang/Optimizer/Dialect/FIRDialect.h
52

registerOpExternalInterfaces?

mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
24–27

Is this really required?

mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
91
104
This revision is now accepted and ready to land.May 17 2023, 1:44 PM

Thanks @agozillon for splitting up the patch.

I think inlining should be fine to do to declare target functions, as the call locations should be occurring in functions or target regions that are being lowered to the appropriate device, so the inlined function should be getting embedded into the appropriate device function.

OK

Would you know whether the link clause will be handled in the OpenMPIRBuilder or would it require the frontend to move the link clause info to target map/target data?

Some of this will at least be handled by the OpenMPIRBuilder during the lowering of declare target's link, I am unfortunately unfamiliar with map/target data and their interactions with link, although it is something I will likely have to look into soon (perhaps after the semantic analysis component of the declare target work has been updated, or soon after). However, at least some of the link case is handled in another two patches (more actually a lot of the IR builder work i've done is purely to support data related declare target cases) I have created:

https://reviews.llvm.org/D149162
https://reviews.llvm.org/D149368

This is an attempt to mimic what Clang generates for the link/to clauses when data is passed to them, creating the relevant metadata and global pointers/references for host and device. However, I am aware that the OpenMP specification mentions that map handles some or all of the data management for declare target link, so I think what these patches do is only part of the work required for supporting the link clause (in part why I call the patch initial).

Thank you very much for the review Kiran, I will apply your comments and try to upstream this on Monday after I'm back from a long weekend break.

flang/include/flang/Optimizer/Dialect/FIRDialect.h
52

Definitely a better name! I'll make the alteration before submission.

mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
24–27

I can double check before I commit upstream, but as I recall a forward declaration is necessary for these, but it's been a little bit so I can't remember the exact reason why and perhaps moving to the interface method allows me to avoid it (as i was using these when it was dialect functions)! So i'll give removing these a try when I'm back from vacation on Monday

I've reverted this till I find a fix for some buildbots it's breaking the compilation for, once I find the fix, I'll re-push and confirm it's all in working order here!

agozillon added a comment.EditedMay 22 2023, 5:42 AM

Re-committed now with a fix to the mlir/lib/Dialect/OpenMP/CMakeLists.txt to add MLIRFuncDialect to the link list, to support the usage of adding the declare target interface to the FuncOp. Will keep track of the buildbots to make sure it's fixed!