Initial support for stand-alone omp target enter data directive without any clauses.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
---|---|---|
623–624 ↗ | (On Diff #434212) | [nit] The other parameters are not described here |
The OpenACC translation for enter data was implemented in the patch https://reviews.llvm.org/D101504. It was subsequently refactored and now is available as https://github.com/llvm/llvm-project/blob/369ce54bb302f209239b8ebc77ad824add9df089/mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp#L510. From what I understand the implementation is more translation heavy and only uses the OpenMPIRBuilder (renamed as OpenACCIRBuilder) for creating the runtime calls. Would a good first step be to move more of the translation code (if possible) to the OpenMPIRBuilder and reuse that rather than starting to implement from scratch?
Also, I see that enter data should at least have one map clause and the OpenACC implementation and Clang (from a quick look) seems to use the tgt_target_data_begin_mapper function. I do not see that runtime function used in this patch.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
---|---|---|
3676 ↗ | (On Diff #434212) | Is this. specific to omp::EnterData operation? You are planning to write <OpenMPIRBuilder::createTargetExitData> for omp::ExitData as well? |
Sure, I will add a test case.
From OpenMP spec map clause is not mandatory that is why I started without mapper function. Also, OMPRTL___tgt_target_data_begin was calling tgt_target_data_begin_mapper in it's definition. So I started with using OMPRTL___tgt_target_data_begin.
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
---|---|---|
623–624 ↗ | (On Diff #434212) | Thanks! I will fix it. |
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
3676 ↗ | (On Diff #434212) | Yes, I am planning to keep them as separate functions for separate directives. |
The restrictions of target enter data in https://www.openmp.org/spec-html/5.0/openmpsu58.html has the following entry,
- At least one map clause must appear on the directive.
yes. At least a unittest that calls createTargetEnterData.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
---|---|---|
3681–3685 ↗ | (On Diff #434212) | OpenMPIRBuilder is part of the llvm namespace, therefore llvm:: qualifiers are unnecessary. |
3691–3692 ↗ | (On Diff #434212) | [style] In the LLVM project, please use the LLVM coding style. |
mlir/include/mlir/Target/LLVMIR/Dialect/Utils/CommonOpenMPOpenACCUtils.h | ||
---|---|---|
1 ↗ | (On Diff #441475) | Update header |
19 ↗ | (On Diff #441475) | Is there something missing or do you want to support only acc::DataOp? |
mlir/lib/Target/LLVMIR/Dialect/Utils/CommonOpenMPOpenACCUtils.cpp | ||
268 ↗ | (On Diff #441475) | Since this is for acc::DataOp it should be in the OpenACC file. |
mlir/include/mlir/Target/LLVMIR/Dialect/Utils/CommonOpenMPOpenACCUtils.h | ||
---|---|---|
19 ↗ | (On Diff #441475) | I think this will be common for both OpenACC and OpenMP acc data is equivalent to omp target data directive which will be used in later patches. |
AFAICT this patch does not do what its title and description say ('omp target enter data' support). Can you update it to reflect what is actually done?
mlir/lib/Target/LLVMIR/Dialect/Utils/CommonOpenMPOpenACCUtils.cpp | ||
---|---|---|
1 ↗ | (On Diff #442632) | Not a big fan of the name of this file. I think ToLLVMIRTranslation should be part of it. maybe DiretiveToLLVMIRTranslation |
79 ↗ | (On Diff #442632) | The functions above are also utilities so I'm not sure this is really needed. |
Thanks for nit pick. I started with getting common code moved from OpenACC to LLVMIR translation to a common place so that it can be shared with both OpenACC and OpenMP.
Move common code from OpenACCToLLVMIRTranslation to Utils/DirectiveToLLVMIRTranslation so that it can be used between OpenACC and OpenMP.
mlir/include/mlir/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.h | ||
---|---|---|
14 | Can you change it to reflect the new name? | |
33 | Missing new line | |
mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp | ||
288 | I'm not sure it makes sense to reuse the part where we process each operands. There is likely no 1 to 1 mapping between OpenMP and OpenACC so it's likely the part that will be specific for each operation. | |
491 | Missing new line. |
mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp | ||
---|---|---|
288 | how about adding a common data interface to both data operations and use that here? |
mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp | ||
---|---|---|
288 | Not sure that will work either. We need to be able to fine tune the flags that are passed for each operands. This will not be common between the two languages for at least some of them. |
mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp | ||
---|---|---|
315 | For example the kHoldFlag here is a specific OpenACC extension so it's wrong to use it for OpenMP. See https://reviews.llvm.org/D106509 and https://reviews.llvm.org/D106510 for references. |
mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp | ||
---|---|---|
288 | I tried moving entire mlir::convertDataOp() function and its supporting utilities back to OpenACCToLLVMIRTranslation.cpp and tried to maintain convertStandaloneDataOp() and some of it's utilities in DirectiveToLLVMIRTranslation.cpp but that change is causing cmake cyclic dependency as each library is dependent on other library. |
mlir/lib/Target/LLVMIR/Dialect/Utils/DirectiveToLLVMIRTranslation.cpp | ||
---|---|---|
288 | Utils should probably not depend on OpenACC/OpenMP translation but the opposite is more likely. |
@raghavendhra @kiranchandramohan After listening to your presentation today, I am inclined towards keeping it separate. There is a big assumption that OpenMP and OpenACC will move hand in hand which might be true now but in the future, it might not be true.
Any pointer to this presentation?
This was discussed today in the OpenMP work for llvm-flang call.
Yeah thank you I could guess that. His comment mentioned a presentation so maybe there is some material to be shared.
@clementval I started a discourse thread https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833 regarding the same discussion which happened in bi-weekly OpenMP in flang technical call. Please let me know your thoughts. Thanks!
Currently waiting for Abid Malik's MLIR Op support for these operations https://reviews.llvm.org/D131915 to land
This is old and no longer needed as this is taken care by latest revision https://reviews.llvm.org/D142914
Can you change it to reflect the new name?