[MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[TIFitis]

Summary

This patch adds OpenMP IRBuilder support for the Target Data directives to allow lowering to LLVM IR.

The mlir::Translation is responsible for generating supporting code for processing the map_operands through the processMapOperand function, and also generate code for the region associated with any Target Data operation.

The OMPIRBuilder is responsible for generating the begin and end mapper function calls.

Limitations:

• use_device_ptr and use_device_addr clauses are NOT supported for Target Data operation.
• nowait clauses are NOT supported for Target Enter and Exit Data operations.
• Only LLVMPointerType is supported for map_operands.

[TIFitis]

@kiranchandramohan I've added the MLIR test as requested. Please let me know if any other changes are required :)

[kiranchandramohan]

Thanks for making the change.
I am still going through the patch, I have a few comments.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1574–1575

SmallVector -> SmallVectorImpl
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4052–4060

There is some recent recommendation to not insert artificial instructions and remove them. The preferred approach is to use splitBB function(s) inside the OpenMPIRBuilder. This function works on blocks without terminators. You can consult the IfCondition code in createParallel.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

4983

Is this a debugging leftover?

4990

Call verifyModule to ensure the IR is well formed.

mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h

14–15

Nit: The header guards would need changing.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1471

Ideally we would not want to create an OpenMP runtime calls in the Translation. This is the job of the OpenMPIRbuilder. I would recommend passing a boolean to the OpenMPIRBuilder and let the IRBuilder pick up the runtime function.

1492

Same comment as above.

mlir/test/Target/LLVMIR/omptarget-llvm.mlir

2

In MLIR it is preferred to test the minimal set of functionalities. So you will have to minimize the CHECK lines and write them manually. Focus on Checking the runtime call and any important IR inserted by the IRBuilder. Keep the contents of the region to a minimum.
https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices

[TIFitis]

Addressed reviewer comments

[TIFitis]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4052–4060

Thanks a lot for taking the time to review this lengthy patch.

This one seems a bit tricky to do. At first glance createParallel seems to be doing something different where its calling different runtime functions based on the IfCondition instead of much in the way of Control Flow changes.

The unreachable inst helps out a lot here as it makes it really easy to keep trace of the resume point for adding instructions after the BodyGen codes are generated.

I am still looking into finding a way to do this elegantly without having to use the unreachable instruction, but would it be a major blocker if we had to keep it?

I have addressed all the other changes you requested.

[kiranchandramohan]

I have some more comments.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1556

Nit: In the following comments either use full stops for all.

It will be helpful if the comments below refer to the OpenMP construct or clauses rather than the MLIR representation. Forr e.g Map Clause instead of Map operand.

1561

Can't the presence/absence of the BodyGenCB indicate the presence/absence of a region?

1567

This should be expanded to say whether it means begin or end mapper based on the value of the boolean and probably also renamed to isBegin or isEnter or something like that.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4052–4060

Thanks for explaining the need for the unreachable. I will leave it with you on whether to make the change.

You can see an instance of a past request for not using temporary instruction. That patch (if for createTask) addressed the issue one way.
https://reviews.llvm.org/D130615#inline-1257711

I gave a quick try and came up with the following code. I don't think it is very elegant, but it is one way to do it.

-  // LLVM utilities like blocks with terminators.
-  auto *UI = Builder.CreateUnreachable();
+  BasicBlock *ContBB = nullptr;
   if (IfCond) {
-    auto *ThenTI =
-        SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
-    ThenTI->getParent()->setName("omp_if.then");
-    Builder.SetInsertPoint(ThenTI);
-  } else {
-    Builder.SetInsertPoint(UI);
+    BasicBlock *EntryBB = Builder.GetInsertBlock();
+    ContBB = splitBB(Builder, /*CreateBranch*/ false);
+    BasicBlock *ThenBB =
+    BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
+                       ContBB->getParent(), ContBB);
+    ContBB->setName(EntryBB->getName() + ".if.end");
+    Builder.CreateCondBr(IfCond, ThenBB, ContBB);
+    Builder.SetInsertPoint(ThenBB);
+    Builder.CreateBr(ContBB);
+    Builder.SetInsertPoint(ThenBB->getTerminator());
   }
 
   ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
@@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createTargetData(
     emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, MapTypesArg,
                    MapNamesArg, MapperAllocas, DeviceID, MapTypeFlags.size());
 
+    BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit");
     BodyGenCB(Builder.saveIP(), Builder.saveIP());
 
-    Builder.SetInsertPoint(UI->getParent());
+    Builder.SetInsertPoint(DataExitBB, DataExitBB->begin());
     // Create call to end the data region.
     emitMapperCall(Builder.saveIP(), endMapperFunc, srcLocInfo, MapTypesArg,
                    MapNamesArg, MapperAllocas, DeviceID, MapTypeFlags.size());
@@ -4100,11 +4104,9 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createTargetData(
                    MapTypeFlags.size());
   }
 
-  // Update the insertion point and remove the terminator we introduced.
-  Builder.SetInsertPoint(UI->getParent());
-  if (IfCond)
-    UI->getParent()->setName("omp_if.end");
-  UI->eraseFromParent();
+  if (ContBB)
+    return {ContBB, ContBB->begin()};
+

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

4915

In general, we have to test more in these unit tests. At the moment this only tests the enter data variant.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Isn't it possible to sink this whole function into the OpenMPIRBuilder by passing it a list of mapOpValue and mapTypeFlags?
lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);

Did i miss something? Or is this in anticipation of more processing required for other types?

1502–1510

Can all this go into the OpenMP IRBuilder? And be passed back if necessary via the callback.

mlir/test/Target/LLVMIR/omptarget-llvm.mlir

50–81

Nit: Reduce this region to the minimum. One or two instructions in the region is sufficient.

[TIFitis]

Adrresed reviewer comments.

[TIFitis]

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1561

The BodyGenCB is always present as an argument right? Passing a nullptr when its not required doesn't seem possible at least when using the BodyGenCallbackTy. Is there a way to selectively pass the BodyGenCB?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4052–4060

I saw the other directives were making use of an explicit ExitBB, but for those directives they always needed to splice the directive into multiple BB's anyways.

Since for target data we don't need to splice the BB unless using the if clause or target region are present, I was trying to find a way to do it without having to add a new BB at all times. IMO adding an unreachable inst that we always remove soon after is better than adding a new BB, but I'd defer to you in this case.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

4915

The IRBuilder part of the code doesn't really generate much code in itself. It mainly calls the BodyGenCB and ProcessMapCB, and makes use of the existing emitMapperCall function.

Test for emitMapperCall is already present in the unit test and the CB functors are tested through the MLIR test. Thus I've only added a single unit test for the IRBuilder, would you like me to add a few more?

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

I'm not fully sure but we might need more MLIR related things when supporting types other than LLVMPointerType. Also there is a call to mlir::LLVM::createMappingInformation.

I guess it might still be possible to move most of it to the IRBuilder, would you like me to do that?

1502–1510

If we decide to move processMapOperand then these can be moved along with it.

[kiranchandramohan]

Thanks for the update and the replies. See comments inline.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1561

The only optional CallBack seems to be the FinalizeCallbackTy. It is defined as an std::function whereas BodyGenCallBackTy is defined as a function_ref. But the definition of function_ref looks like it can be used to check whether it is a nullptr or not. Did you face any issues in trying to make it optional with a default parameter value of nullptr?

https://llvm.org/doxygen/STLFunctionalExtras_8h_source.html#l00036

void emitCancelationCheckImpl(Value *CancelFlag,
                              omp::Directive CanceledDirective,
                              FinalizeCallbackTy ExitCB = {});

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4052–4060

OK. You can retain the unreachable for now.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

4915

Yes, please add a test with a region and for exit data as well.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Callbacks are useful when there is frontend-specific handling that is required. If more types require to be handled then it is better to have the callback. We can revisit this after all types are handled. I assume, the current handling is for scalars and arrays of known-size.

[TIFitis]

Addressed comments. Rebased.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

1561

Sorry, I was being stupid and trying to pass nullptr as an argument instead of making the actual parameter optional. BodyGenCallBackTy works.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

4052–4060

Thanks. I've added a small comment in the code to explain what the UI is used for.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

I am a novice at FORTRAN so I'm not aware of all the types and scenarios.

I've tested the following cases and they work end-to-end:

Fortran:

subroutine openmp_target_data_region(a)
    real :: a(*)
    integer :: b(1024)
    character :: c
    integer, pointer :: p
    !$omp target enter data map(to: a, b, c, p)
end subroutine openmp_target_data_region

LLVM IR( flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll ):

; ModuleID = 'FIRModule'
source_filename = "FIRModule"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%struct.ident_t = type { i32, i32, i32, i32, ptr }

@0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
@1 = private unnamed_addr constant [56 x i8] c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
@2 = private unnamed_addr constant [56 x i8] c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
@3 = private unnamed_addr constant [56 x i8] c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
@4 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1
@5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 22, ptr @4 }, align 8
@.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, i64 1, i64 1, i64 1]
@.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr @2, ptr @3]

declare ptr @malloc(i64)

declare void @free(ptr)

define void @openmp_target_data_region_(ptr %0) {
  %2 = alloca [4 x ptr], align 8
  %3 = alloca [4 x ptr], align 8
  %4 = alloca [4 x i64], align 8
  %5 = alloca [1024 x i32], i64 1, align 4
  %6 = alloca [1 x i8], i64 1, align 1
  %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
  %8 = alloca ptr, i64 1, align 8
  store ptr null, ptr %8, align 8
  br label %entry

entry:                                            ; preds = %1
  %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
  store ptr %0, ptr %9, align 8
  %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
  store ptr %0, ptr %10, align 8
  %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
  store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %11, align 8
  %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
  store ptr %5, ptr %12, align 8
  %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
  store ptr %5, ptr %13, align 8
  %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
  store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %14, align 8
  %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
  store ptr %6, ptr %15, align 8
  %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
  store ptr %6, ptr %16, align 8
  %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
  store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %17, align 8
  %18 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 3
  store ptr %7, ptr %18, align 8
  %19 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 3
  store ptr %7, ptr %19, align 8
  %20 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 3
  store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %20, align 8
  %21 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
  %22 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
  %23 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
  call void @__tgt_target_data_begin_mapper(ptr @5, i64 -1, i32 4, ptr %21, ptr %22, ptr %23, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
  ret void
}

; Function Attrs: nounwind
declare void @__tgt_target_data_begin_mapper(ptr, i64, i32, ptr, ptr, ptr, ptr, ptr, ptr) #0

; Function Attrs: nounwind
declare void @__tgt_target_data_end_mapper(ptr, i64, i32, ptr, ptr, ptr, ptr, ptr, ptr) #0

attributes #0 = { nounwind }

!llvm.module.flags = !{!0}

!0 = !{i32 2, !"Debug Info Version", i32 3}

If I am missing some important types here then please let me know, I'll try to see if they work and if not I'll add support for them in further patches.

[kiranchandramohan]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

In general how are you passing the size of the fortran variable/type to the OpenMP runtime? For scalars and arrays with sizes known at compile time, this comes from the type itself. But for other types like assumed-shape arrays, variable length arrays this information comes from the descriptor or from other fields. My question is how is this being collected and passed to the runtime?

For all the types, I see the following code in the IR you gave above for generating the ArgSizes argument of __tgt_target_data_begin_mapper. I don't understand how the code (and size) be the same for all the types.

...
%11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %11, align 8
...
%14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %14, align 8
...
%17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %17, align 8
...
%20 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 3
store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %20, align 8
...
%23 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
call void @__tgt_target_data_begin_mapper(ptr @5, i64 -1, i32 4, ptr %21, ptr %22, ptr %23, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)

I would like some more clarity on this before proceeding. Clang generates different code for this and I see that it is appropriately filling the ArgSizes field.

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

OpenMPIRBuilder::getSizeInBytes is the function responsible for calculating the ArgSizes.

For the Value : %1 = alloca i64, i64 1, align 8 it returns size as i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64) and TBH I don't understand this. This function was taken from OpenACC.

I will re-implement this function and update the patch.

[TIFitis]

Added names for offload mapper variables.

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Actually, the generated code seems correct. The first answer here gives insight into how OpenMPIRBuilder::getSizeInBytes is calculating the size of the type.

Opaque pointers make it look the same for all the different types, disabling opaque pointers you get something like the following:

integer(8) :: a :

%7 = getelementptr inbounds [1 x i64], [1 x i64]* %.offload_sizes, i32 0, i32 0
store i64 ptrtoint (i64** getelementptr (i64*, i64** null, i32 1) to i64), i64* %7, align 4

integer :: b(1024) :

%8 = getelementptr inbounds [1 x i64], [1 x i64]* %.offload_sizes, i32 0, i32 0
store i64 ptrtoint ([1024 x i32]** getelementptr ([1024 x i32]*, [1024 x i32]** null, i32 1) to i64), i64* %8, align 4

Let me know if this makes sense.

Thanks,
Akash

[kiranchandramohan]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Ahh OK. It does make it a bit harder to read.

But going to back to my general question:

In general how are you passing the size of the fortran variable/type to the OpenMP runtime? For scalars and arrays with sizes known at compile time, this comes from the type itself. But for other types like assumed-shape arrays, variable length arrays this information comes from the descriptor or from other fields. My question is how is this being collected and passed to the runtime

Consider the following two subroutines, It has two assumed shape arrays. In sb0 it is a rank-1 array, in sb1 it is a rank-2 array. At the llvm dialect layer, these two will be represented by struct equivalents of Fortran descriptors as given below. If we now find the size of the types, it would get the size of the descriptor struct rather than it memory it is referring to. I guess this is not what we want to do. I believe this would require some special processing, unless the patch also does something for this.

omp.target_data   map((to -> %arg0 : !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)>>))

omp.target_data   map((to -> %arg0 : !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8, array<1 x array<3 x i64>>)>>))

subroutine sb0(a)
   integer :: a(:)
   !$omp target data map(to: a)
   a(10) = 20
   !$omp end target data
end subroutine

subroutine sb1(a)
   integer :: a(:,:)
   !$omp target data map(to: a)
   a(5,6) = 20
   !$omp end target data
end subroutine

[kiranchandramohan]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Just want to clarify that I am not expecting a fix here. But just a statement about what is supported and what is not supported.

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

In line 1382 you can see how the size is determined. It is done simply by passing the map variable to the getSizeInBytes function.

I haven't added any special handling for assumed-shape arrays or any other special cases. I am not sure if it is equivalent to an int* in C, but looking at the llvm IR, if the map variable is an i32* then clang sets the size to 4, and for assumed-shape arrays in your examples sb0 and sb1 it also creates i32* in the llvm IR and the size for these calculated in the same way.

I am not sure if this is the correct behaviour. Do you know what is the correct size that should be passed in your examples to the runtime? If it is relatively straightforward then I can look into adding support for these, otherwise maybe we can add a TODO for handling assumed-shape arrays in a future patch.

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Sorry, made a mistake in previous comment.

Clang sets size to 8 and not 4 for i32*.

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

Also, the standard specifies assumed-size arrays are not supported. I am not sure if the arrays in your example are assumed-shape or assumed-size.

[kiranchandramohan]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

They are assumed-shape arrays.

We are probably missing a semantic check for the assumed-size case. But good to know that they need not be supported. In one of the examples, you provided, it was mentioned to be supported. If you can create a ticket for this that would be great. Gfortran catches this error.

subroutine openmp_target_data_region(a)
    real :: a(*)
    !$omp target enter data map(to: a)
end subroutine openmp_target_data_region

In general, besides assumed-shape, there are various cases where a descriptor can come in. Like for pointers and allocatable arrays.

subroutine sb4
   integer, pointer :: a(:)
   allocate(a(10))
   !$omp target data map(to: a)
   a(10) = 20
   !$omp end target data
end subroutine

For array sections:

subroutine sb4
   integer :: a(10)
   !$omp target data map(to: a(3:10))
   a(10) = 20
   !$omp end target data
end subroutine

I believe the size should be passed to the runtime, then only it can compute how many bytes should be mapped.

From your answers,

1. The following types are supported:

scalars, constant sized arrays

2. The following types are probably supported:

variable length arrays, derived types with elements of type in (1) or (2)

3. The following types are not supported:

assumed-size arrays, pointers, allocatables, derived type with elements of type in (3), array-sections (this segfaults now).

Would it be OK to add types in (2) and (3) as TODO failures in flang?

[TIFitis]

Added TODO for unsupported map operand types in Flang frontend

[TIFitis]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1357

I've added the TODO. Please let me know if I missed any scenario there.

Thanks :)

[TIFitis]

Fix test

[kiranchandramohan]

LGTM. Please wait a day before submission. Move the flang portion to a separate commit.

flang/lib/Lower/OpenMP.cpp

727 ↗

(On Diff #505456)

Nit: expand auto.

727–738 ↗

(On Diff #505456)

These changes should go in a separate patch.

737 ↗

(On Diff #505456)

Since TODO will print a "Not yet implemented" message, I think we can remove the trailing "not supported suffix".

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1502–1510

Now that we decided not to move processMapOperand, can this be moved to the OpenMPIRBuilder?

1523

Nit: I think this should be an assertion.

[TIFitis]

Addresed reviewer comments. Removed flang changes.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1502–1510

Hi, I am working on a patch to add use_device_ptr and use_deice_addr clauses. As part of that I have restructured how mapOperands are processed and separated the mlir and codegen part of it. These are already moved to OMPIRBuilder in that patch.

1523

AFAIK the mlir op for this only allows for one region.