This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by TIFitis on Jan 30 2023, 10:41 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
clementval added inline comments.Jan 30 2023, 12:20 PM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

Instead of copy pasting this from mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp can you extract it and put it in a common shared file so bith translation can use the same code without duplication?

kiranchandramohan added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

@raghavendhra put up a patch some time back and he faced some issues. It might be good to check with him or may be he can comment here.
https://reviews.llvm.org/D127037
https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833

clementval added inline comments.Jan 31 2023, 1:36 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

Just moving the three functions should be trivial. I'm not talking about the processMapOperand.

TIFitis marked 2 inline comments as done.Jan 31 2023, 3:14 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

I've moved getSizeInBytes.

The other two functions make use of mlir::Location and thus can't be moved trivially.

I can still try to move them by individually passing the elements of mlir::Location but that might not be ideal. Is that what you'd like?

TIFitis updated this revision to Diff 493549.Jan 31 2023, 3:31 AM
TIFitis marked an inline comment as done.

Moved getSizeInBytes to OMPIRBuilder.cpp

clementval added inline comments.Jan 31 2023, 3:51 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

What about a new header file in mlir/include/mlir/Target/LLVMIR/Dialect/** shared by mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp and mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp. That should be doable.

TIFitis marked an inline comment as done.Jan 31 2023, 5:22 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp and mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp already have access to the common mlir::Location type.

Problem is that OMPIRBuilder.cpp is the only common file between them where I can move the two functions to. Currently there are no mlir/** include files in OMPIRBuilder.cpp and it seems to me like a strict design choice to have it that way.

clementval added inline comments.Jan 31 2023, 5:40 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

The functions can be header only. Why do you need to put them in the OMPIRBuilder.cpp? I think it is better than duplicate the exact same code over.

TIFitis updated this revision to Diff 493615.Jan 31 2023, 7:41 AM
TIFitis marked an inline comment as done.

Added new header file mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h, moved two common functions to the new file.

TIFitis marked an inline comment as done.Jan 31 2023, 7:43 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

Sorry, I misunderstood you earlier.
I've added a new header file mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h, this is my first attempt at adding a new header file, please let me know if you find any issues.

clementval added inline comments.Jan 31 2023, 11:20 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

Thanks! That's what I had in mind. We might want to check with MLIR folks if mlir::utils is suited for that. I don't mind if it is mlir::omp::builder or smth similar since it is related to the OMPIRBuilder.

TIFitis marked 2 inline comments as done.Feb 1 2023, 8:16 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

Since the utils file is common to all the dialects I kept it as mlir::utils.

How do I get the opinion from people working in MLIR on this, can you suggest some reviewers whom I can add?

clementval added inline comments.Feb 2 2023, 5:55 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

It's only valid for translation to the llvmir dialect so that why mlir::utils seems to generic to me.

Maybe @ftynse has some thoughts on this.

TIFitis marked an inline comment as done.Feb 2 2023, 6:06 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

I agree with you on that, would perhaps renaming it to something like mlir::dialect-utils be a better option?

Please add more description to the summary regarding the scope of this patch. If it is only able to lower specific llvm-dialect types, please call out that. Please also explain the split in work between mlir::Translation and OpenMPIRbuilder. You can also consider splitting this into two patches. One that just adds the changes to OpenMPIRBuilder and the child version of the patch adding the translation in MLIR.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396

You can post in MLIR discourse MLIR section (https://discourse.llvm.org/c/mlir/31) to get an opinion.

open-directive-utils , ompbuilder-utils are other options. Simialr names could be considered for the file name as well.

TIFitis marked an inline comment as done.Feb 6 2023, 10:14 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1356–1396
ftynse added inline comments.Feb 7 2023, 1:22 AM
mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
13 ↗(On Diff #493615)

This tag is wrong. It should be MLIR_TARGET_LLVMIR_DIALECT_UTILS_H.

59 ↗(On Diff #493615)

Nit: please add a newline.

ftynse added inline comments.Feb 7 2023, 1:24 AM
mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h
27 ↗(On Diff #493615)

It's not great to have static functions in a header. I suppose this is done to avoid build dependencies, but it's better to get the dependencies right.

TIFitis updated this revision to Diff 495493.Feb 7 2023, 5:46 AM
TIFitis marked 4 inline comments as done.

Changed namespace to mlir::LLVM for common files. Addressed reviewer comments.

TIFitis edited the summary of this revision. (Show Details)Feb 7 2023, 7:14 AM
TIFitis updated this revision to Diff 495530.Feb 7 2023, 7:16 AM
TIFitis edited the summary of this revision. (Show Details)

Added explicit failure for use_device_ptr and use_device_addr clauses.

Please avoid the MLIR style in any other subproject. I know some of the OpenMP Builder stuff already uses llvm:: and MLIR style names, but that is in itself bad, not something we should extend.

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

Nit: Style, no llvm::

1582

Style, see above.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4113

Style, see above.

4172

Style, see above.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
4991

Style, see above.

TIFitis updated this revision to Diff 495864.Feb 8 2023, 8:27 AM
TIFitis marked 5 inline comments as done.

Addressed reviewer comments

TIFitis updated this revision to Diff 497045.Feb 13 2023, 10:42 AM

Fixing build errors. Added mlir/lib/Target/LLVMIR/Dialect/Utils.cpp, made it part of the MLIRTargetLLVMIRExport lib.

This revision is now accepted and ready to land.Feb 16 2023, 10:05 AM

@kiranchandramohan @clementval can you please take a look and let me know if any more changes are required for this patch?

Thanks,
Akash

kiranchandramohan requested changes to this revision.Feb 22 2023, 6:59 AM

Please add tests for the MLIR portion.

Could you also post the full LLVM IR for a construct with the map clause?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4163

Nit:Remove empty line.

mlir/lib/Target/LLVMIR/Dialect/Utils.cpp
1 ↗(On Diff #497045)

Nit: I would prefer the OpenMPCommon.cpp name suggested in https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221.

This revision now requires changes to proceed.Feb 22 2023, 6:59 AM
TIFitis marked 4 inline comments as done.Feb 22 2023, 10:47 AM

Please add tests for the MLIR portion.

Can you please tell me where to add these?

Could you also post the full LLVM IR for a construct with the map clause?

The following is a simple example, let me know if you'd like a more complex example with loop :)

Flang:

subroutine openmp_target_data
    integer :: i
    !$omp target data map(tofrom: i)
        i = 99;
    !$omp end target data
end subroutine openmp_target_data

FIR:

func.func @_QPopenmp_target_data() {
  %0 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFopenmp_target_dataEi"}
  omp.target_data   map((tofrom -> %0 : !fir.ref<i32>)) {
    %c99_i32 = arith.constant 99 : i32
    fir.store %c99_i32 to %0 : !fir.ref<i32>
    omp.terminator
  }
  return
}

LLVMIR:

llvm.func @_QPopenmp_target_data() {
  %0 = llvm.mlir.constant(1 : i64) : i64
  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, operand_segment_sizes = array<i32: 0, 0>, uniq_name = "_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr<i32>
  omp.target_data   map((tofrom -> %1 : !llvm.ptr<i32>)) {
    %2 = llvm.mlir.constant(99 : i32) : i32
    llvm.store %2, %1 : !llvm.ptr<i32>
    omp.terminator
  }
  llvm.return
}

llvm IR .ll:

; ModuleID = 'LLVMDialectModule'
source_filename = "LLVMDialectModule"
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 = "aarch64-unknown-linux-gnu"

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

@0 = private unnamed_addr constant [26 x i8] c";test.mlir;unknown;4;10;;\00", align 1
@1 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1
@2 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 22, ptr @1 }, align 8
@.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 3]
@.offload_mapnames = private constant [1 x ptr] [ptr @0]

declare ptr @malloc(i64)

declare void @free(ptr)

define void @_QPopenmp_target_data() {
  %1 = alloca [1 x ptr], align 8
  %2 = alloca [1 x ptr], align 8
  %3 = alloca [1 x i64], align 8
  %4 = alloca i32, i64 1, align 4
  br label %entry

entry:                                            ; preds = %0
  %5 = getelementptr inbounds [1 x ptr], ptr %1, i32 0, i32 0
  store ptr %4, ptr %5, align 8
  %6 = getelementptr inbounds [1 x ptr], ptr %2, i32 0, i32 0
  store ptr %4, ptr %6, align 8
  %7 = getelementptr inbounds [1 x i64], ptr %3, i32 0, i32 0
  store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr %7, align 8
  %8 = getelementptr inbounds [1 x ptr], ptr %1, i32 0, i32 0
  %9 = getelementptr inbounds [1 x ptr], ptr %2, i32 0, i32 0
  %10 = getelementptr inbounds [1 x i64], ptr %3, i32 0, i32 0
  call void @__tgt_target_data_begin_mapper(ptr @2, i64 -1, i32 1, ptr %8, ptr %9, ptr %10, ptr @.offload_maptypes, ptr @.offload_mapnames, ptr null)
  br label %omp.data.region

omp.data.region:                                  ; preds = %entry
  store i32 99, ptr %4, align 4
  br label %omp.region.cont

omp.region.cont:                                  ; preds = %omp.data.region
  %11 = getelementptr inbounds [1 x ptr], ptr %1, i32 0, i32 0
  %12 = getelementptr inbounds [1 x ptr], ptr %2, i32 0, i32 0
  %13 = getelementptr inbounds [1 x i64], ptr %3, i32 0, i32 0
  call void @__tgt_target_data_end_mapper(ptr @2, i64 -1, i32 1, ptr %11, ptr %12, ptr %13, 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}

Cheers,
Akash

mlir/lib/Target/LLVMIR/Dialect/Utils.cpp
1 ↗(On Diff #497045)

Would you also like me to move the file inside OpenMP ( mlir/lib/Target/LLVMIR/Dialect/OpenMP ) ?

TIFitis updated this revision to Diff 499583.Feb 22 2023, 10:48 AM
TIFitis marked an inline comment as done.

Addressed reviewer comments, fixed error in MLIR translation.

Please add tests for the MLIR portion.

Can you please tell me where to add these?

In mlir/test/Target/LLVMIR/openmp-llvm.mlir. You can also consider starting a new file for target constructs in that directory.

mlir/lib/Target/LLVMIR/Dialect/Utils.cpp
1 ↗(On Diff #497045)

No, the current location is fine.

TIFitis updated this revision to Diff 500807.Feb 27 2023, 8:31 AM

Added MLIR test.

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

kiranchandramohan requested changes to this revision.Mar 3 2023, 5:36 AM

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

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1576–1577
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4055–4063

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

This revision now requires changes to proceed.Mar 3 2023, 5:36 AM
TIFitis updated this revision to Diff 502175.Mar 3 2023, 10:18 AM
TIFitis marked 7 inline comments as done.

Addressed reviewer comments

TIFitis added inline comments.Mar 3 2023, 10:27 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4055–4063

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 requested changes to this revision.Mar 6 2023, 3:47 AM

I have some more comments.

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

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.

1563

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

1569

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
4055–4063

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.

This revision now requires changes to proceed.Mar 6 2023, 3:47 AM
TIFitis updated this revision to Diff 502670.Mar 6 2023, 8:40 AM
TIFitis marked 9 inline comments as done.

Adrresed reviewer comments.

TIFitis added inline comments.Mar 6 2023, 8:42 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1563

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
4055–4063

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.

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

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

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
4055–4063

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 updated this revision to Diff 503137.Mar 7 2023, 12:58 PM
TIFitis marked 3 inline comments as done.

Addressed comments. Rebased.

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

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
4055–4063

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 requested changes to this revision.Mar 8 2023, 8:25 AM
kiranchandramohan added inline comments.
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.

This revision now requires changes to proceed.Mar 8 2023, 8:25 AM
TIFitis marked an inline comment as done.Mar 9 2023, 5:08 AM
TIFitis added inline comments.
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 updated this revision to Diff 503741.Mar 9 2023, 5:50 AM

Added names for offload mapper variables.

TIFitis added inline comments.Mar 9 2023, 6:07 AM
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

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
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 marked 2 inline comments as done.Mar 13 2023, 9:36 AM
TIFitis added inline comments.
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 marked an inline comment as done.Mar 13 2023, 9:56 AM
TIFitis added inline comments.
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 added inline comments.Mar 13 2023, 10:02 AM
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.

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

  1. The following types are probably supported:

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

  1. 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 updated this revision to Diff 505116.Mar 14 2023, 8:29 AM

Added TODO for unsupported map operand types in Flang frontend

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 8:29 AM
TIFitis marked an inline comment as done.Mar 14 2023, 8:31 AM
TIFitis added inline comments.
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 updated this revision to Diff 505456.Mar 15 2023, 5:59 AM
TIFitis marked an inline comment as done.

Fix test

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.

This revision is now accepted and ready to land.Mar 17 2023, 3:21 AM
TIFitis updated this revision to Diff 506054.Mar 17 2023, 5:48 AM
TIFitis marked 5 inline comments as done.

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.

mehdi_amini added inline comments.Apr 21 2023, 8:03 PM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1533

mapperFunc is used uninitialized here which is UB, can you look into this?

TIFitis added inline comments.Apr 22 2023, 1:37 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1533

Thanks for pointing out, I'll push a patch to fix this.

TIFitis added inline comments.Apr 22 2023, 2:54 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1533
mehdi_amini added inline comments.Apr 22 2023, 8:47 AM
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1491

This line is not needed after the fix you pushed right?

Seems like we could also just set bool mapperFunc = isa<omp::EnterDataOp>(op); or something like that.

1533

Thanks for the quick fix!

TIFitis marked 3 inline comments as done.Apr 24 2023, 4:48 AM
TIFitis added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1491

I've already removed this line in my fix.

Yes, I think we can get rid of the variable altogether and just pass the isa as an argument when calling. I'll add this change in one of my ongoing patches.