LGTM :)

Updated test with correct function attributes.

LGTM

In D146648#4554063, @kiranchandramohan wrote:

use_device_ptr: The mapped variables marked with use_device_ptr are accessed through new pointer allocas inside the region.
use_device_addr: The mapped variables marked with use_device_addr are accessed through the base pointer mappers.

This explanation in the summary talks of mapped variables. Mapping is always not necessary. Could you also expand the explanation a little more? And also talk a bit about the difference between use_device_ptr and use_device_addr.

Ping for review :)

In D146648#4531472, @kiranchandramohan wrote:

BTW I am not sure how to resolve the windows build error that I am getting repeatedly. It's giving an unknown identifier error for the fail functor being called at OpenMPToLLVMIRTranslation.cpp:1429. Any insight would be much appreciated.

Might be a bug in the compiler in windows. You are trying to call a lambda function inside a lambda function here. Try moving the second lambda into a static function or try capturing the second lambda explicitly in the first lambda.

LGTM :)

Addressed reviewer comments. Try to fix build error.

BTW I am not sure how to resolve the windows build error that I am getting repeatedly. It's giving an unknown identifier error for the fail functor being called at OpenMPToLLVMIRTranslation.cpp:1429. Any insight would be much appreciated.

In D146648#4527573, @kiranchandramohan wrote:

While looking into this patch, I noticed that the current implementation in Semantics and MLIR requires the MAP clause to always be present. But the standard only says the following,

At least one map, use_device_addr or use_device_ptr clause must appear on the directive.

I will fix this. But this made having a look at the code generated only with use_device_addr, use_device_ptr and without map difficult.

Addressed reviewer comments. Added missing LoadInst for use_device_addr inside region block.

Attempt at fixing build error

Renamed failure() to fail()

Rebased. Addressed reviewer comments. Fixed bug in generating map info for device_addr - device info type was incorrectly being set to pointer.

LGTM.

Please allow for another +1.

@NimishMishra Can you please address the previous comments. I'd be happy to review it further.

Ping for review

Here is an example of what clang generates:

Ping for review :)

Ping for review :)

Rebased

Addressed reviewer comments.

Rebased to fix build error.

Addressed reviewer comments

Made changes to use updated OMPIRBuilder

Gentle ping :)

Moved device clause emition after if clause.

Addressed reviewer comments.

Addressed reviewer comments

Fixed support for pointer type.

@kiranchandramohan Thanks a lot for helping me with the pseudocode.

Added block arguments.

I've updated the patch. I am not sure if this is the correct way of adding the block arguments.

[WIP] Add block arguments.

In D152824#4432759, @kiranchandramohan wrote:

In D152824#4432675, @TIFitis wrote:

My current implementation generates the following FIR.

Fortran:

subroutine omp_target_data
    integer :: a
    a = 10
    !$omp target data map(tofrom: a) use_device_ptr(a)
        a = 20
    !$omp end target data
    a = 30
end subroutine omp_target_data

FIR:

func.func @_QPomp_target_data() {
  %0 = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFomp_target_dataEa"}
  %c10_i32 = arith.constant 10 : i32
  fir.store %c10_i32 to %0 : !fir.ref<i32>
  %1 = fir.alloca i32 {bindc_name = "a", pinned, uniq_name = "_QFomp_target_dataEa"}
  omp.target_data   map((tofrom -> %0 : !fir.ref<i32>))   use_device_ptr((%1 -> %0 : !fir.ref<i32>)) {
    %c20_i32 = arith.constant 20 : i32
    fir.store %c20_i32 to %1 : !fir.ref<i32>
    omp.terminator
  }
  %c30_i32 = arith.constant 30 : i32
  fir.store %c30_i32 to %0 : !fir.ref<i32>
  return
}

Is the above code more or less what you are expecting?

No. What I was thinking was something like the following.

func.func @_QPomp_target_data() {
  %0 = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFomp_target_dataEa"}
  %c10_i32 = arith.constant 10 : i32
  fir.store %c10_i32 to %0 : !fir.ref<i32>
  omp.target_data   map((tofrom -> %0 : !fir.ref<i32>))   use_device_ptr((%1 -> %0 : !fir.ref<i32>)) {
  ^bb0(%1: !fir.ref<i32>):
    %c20_i32 = arith.constant 20 : i32
    fir.store %c20_i32 to %1 : !fir.ref<i32>
    omp.terminator
  }
  %c30_i32 = arith.constant 30 : i32
  fir.store %c30_i32 to %0 : !fir.ref<i32>
  return
}

However, since MLIR would also be creating private versions for us now, we would have to add in code in the IRBuilder to treat clang and mlir separately which I'd like to avoid as much as possible.

MLIR will not be creating a private version. It will only be providing a block argument that you can use to lower easily to the private version created by the IRBuilder. (I believe, you previously reported that it was easy to use block arguments to lower to LLVM IR.)

Also, my initial plan was to have OMPIRBuilder entirely handle codegen for both clang and mlir.

My current implementation generates the following FIR.

In D152824#4427174, @kiranchandramohan wrote:

Thinking of this again, we might not need a change in the operands for use_device_ptr or use_device_addr since we are only adding block arguments.

Rebased

Addressed reviewer comments

In D152824#4421288, @kiranchandramohan wrote:

In D152824#4420853, @TIFitis wrote:

In D152824#4418735, @kiranchandramohan wrote:

Weren't you planning to make the device_ptr a block argument operand?

I made that change when lowering to llvm IR in mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Here's a snippet of code showing how I did it. If there's a better way of doing this I'd be happy to know.

for (auto &devPtrOp : useDevPtrOperands) {
  llvm::Value *mapOpValue = moduleTranslation.lookupValue(devPtrOp);
  const auto &arg =
      region.addArgument(devPtrOp.getType(), devPtrOp.getLoc());
  moduleTranslation.mapValue(arg,
                             info.DevicePtrInfoMap[mapOpValue].second);
  replaceAllUsesInRegionWith(devPtrOp, arg, region);
}

In D152824#4420853, @TIFitis wrote:

In D152824#4418735, @kiranchandramohan wrote:

Weren't you planning to make the device_ptr a block argument operand?

I made that change when lowering to llvm IR in mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
Here's a snippet of code showing how I did it. If there's a better way of doing this I'd be happy to know.

for (auto &devPtrOp : useDevPtrOperands) {
  llvm::Value *mapOpValue = moduleTranslation.lookupValue(devPtrOp);
  const auto &arg =
      region.addArgument(devPtrOp.getType(), devPtrOp.getLoc());
  moduleTranslation.mapValue(arg,
                             info.DevicePtrInfoMap[mapOpValue].second);
  replaceAllUsesInRegionWith(devPtrOp, arg, region);
}

The modelling for the Operation (target data) should be changed to represent both use_device_addr and use_device_addr as a block argument.
May be something like omp.target_data use_device_addr(%daddr -> %a), where %daddr is an entry block argument and %daddr will be used inside the region and not %a.
Note that this might require a custom printer and parser.
This will be a more accurate modelling since the standard explicitly says that the address will be a device address (and not the host address) and hence it is incorrect to use the host address in the body of the region.

The device_addr example from your test will look like the following.

func.func @_QPomp_target_device_addr() {
  %0 = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "a", uniq_name = "_QFomp_target_device_addrEa"}
  %1 = fir.zero_bits !fir.ptr<i32>
  %2 = fir.embox %1 : (!fir.ptr<i32>) -> !fir.box<!fir.ptr<i32>>
  fir.store %2 to %0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
  omp.target_data   map((tofrom -> %0 : !fir.ref<!fir.box<!fir.ptr<i32>>>))   use_device_addr(%daddr -> %0 : !fir.ref<!fir.box<!fir.ptr<i32>>>) {
    %c10_i32 = arith.constant 10 : i32
    %3 = fir.load %daddr : !fir.ref<!fir.box<!fir.ptr<i32>>>
    %4 = fir.box_addr %3 : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
    fir.store %c10_i32 to %4 : !fir.ptr<i32>
    omp.terminator
  }
  return
}

While lowering to LLVM, you can hence do this in fewer steps.

for (auto &devPtrOp : useDevPtrOperands) {
  llvm::Value *mapOpValue = moduleTranslation.lookupValue(devPtrOp);
  const auto &arg = getTheArgumentAssociatedWith(devPtrOp)
  moduleTranslation.mapValue(arg,
                             info.DevicePtrInfoMap[mapOpValue].second);
}

For handling entry block arguments, you can refer to both the wsloop op or the atomic update op.

Fixed typo

Rebased

In D152824#4418735, @kiranchandramohan wrote:

Weren't you planning to make the device_ptr a block argument operand?

Rebased

Update MLIR BodyGen callback.

Addressed reviewer comments.

Rebased

Rebased

Addressed reviewer comments.