[PATCH] D127037: [OpenMP][IRBuilder] 'omp target enter data' support.

[Raghu via Phabricator via llvm-commits]

raghavendhra added a comment.

In D127037#3558172 <https://reviews.llvm.org/D127037#3558172>, @shraiysh wrote:

> Please add testcase in `llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp`



In D127037#3558523 <https://reviews.llvm.org/D127037#3558523>, @abidmalikwaterloo wrote:

> Do we need test cases for this patch?

Sure, I will add a test case.

In D127037#3558292 <https://reviews.llvm.org/D127037#3558292>, @kiranchandramohan wrote:

> 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.

>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.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:623-624
+  ///
+  /// \param Loc The location where the target enter data directive was
+  /// encountered.
+  void createTargetEnterData(const LocationDescription &Loc, Value *Device,
----------------
shraiysh wrote:
> [nit] The other parameters are not described here
Thanks! I will fix it.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3676
+void OpenMPIRBuilder::createTargetEnterData(
+    const LocationDescription &Loc, Value *Device, Value *IfConditionValue,
+    Value *NumDependences, Value *DependenceAddress, bool HaveNowaitClause) {
----------------
abidmalikwaterloo wrote:
> Is this. specific to omp::EnterData operation?  You are planning to write <OpenMPIRBuilder::createTargetExitData> for omp::ExitData as well?
Yes, I am planning to keep them as separate functions for separate directives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127037/new/

https://reviews.llvm.org/D127037