This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Add Lowering support for OpenMP Target Data, Exit Data and Enter Data directives
ClosedPublic

Authored by TIFitis on Jan 23 2023, 6:33 AM.

Details

Summary

This patch adds Fortran Lowering support for the OpenMP Target Data, Target Exit Data and Target Enter Data constructs.
operation.

Diff Detail

Event Timeline

TIFitis created this revision.Jan 23 2023, 6:33 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
TIFitis requested review of this revision.Jan 23 2023, 6:33 AM
clementval added inline comments.Jan 23 2023, 6:40 AM
flang/lib/Lower/OpenMP.cpp
568

Same comment as below for the currentLocation.

571–574

In the lowering we use fully qualified name for stuff from mlir and llvm. Can you update the code here and in other places?

685

Spell out auto here.

685

Becareful, the current location is probably set to the last Fortran construct and not on the openmp directive itself. Look at https://reviews.llvm.org/D131659 to see how we did it for OpenACC.

TIFitis updated this revision to Diff 491352.Jan 23 2023, 6:46 AM

Small fixes. Fixed linting.

TIFitis updated this revision to Diff 491418.Jan 23 2023, 9:20 AM

Added tests. Addressed reviewer comments.

TIFitis marked 4 inline comments as done.Jan 23 2023, 9:22 AM
TIFitis added inline comments.
flang/lib/Lower/OpenMP.cpp
685

I've tried to fix this issue in both places. Please let me know if it's still wrong.

kiranchandramohan requested changes to this revision.Jan 23 2023, 4:02 PM

Looks mostly fine. Please see comments inline.

flang/lib/Lower/OpenMP.cpp
599

Is that release for exit and alloc for enter/data?
Could you give a pointer to the code?

607–609

Nit: Braces are not required.

610

Any specific reason for not including close and present?

658

Nit: Add an assert or a TODO unhandled for the default else.

679

Nit: Add an assert or a TODO unhandled for the default else.

flang/test/Lower/target_data.f90
106 ↗(On Diff #491418)

Nit: No newline.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
152

There are no tests for the contents of this file. Move this to a separate patch.

This revision now requires changes to proceed.Jan 23 2023, 4:02 PM
clementval added inline comments.Jan 23 2023, 11:57 PM
flang/lib/Lower/OpenMP.cpp
685

You need to create a new location from the source of the clause or the directive. Something like:

mlir::Location clauseLocation = converter.genLocation(clause.source);
TIFitis updated this revision to Diff 491696.Jan 24 2023, 3:40 AM
TIFitis marked 9 inline comments as done.

Addressed reviewer comments.

TIFitis added inline comments.Jan 24 2023, 4:52 AM
flang/lib/Lower/OpenMP.cpp
599

Sorry, what do you mean by give a pointer to the code?

I've expanded the comment to mention alloc is for target data and enter data, and release is for exit data.

610

Parser support is not present.

flang/test/Lower/target_data.f90
106 ↗(On Diff #491418)

If you see the raw file there is no newline at the end of file.

I don't know why the "No newline at end of file" is shown here.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
152

I've remove these changes.

It enabled conversion from FIR Dialect to LLVM Dialect.

Can you please tell me where to add tests for this?

flang/lib/Lower/OpenMP.cpp
599

Sorry, I meant a pointer to the runtime code.

Ideally, we should not assume anything about the OpenMP runtime. We are lowering to the OpenMP dialect here. Any such assumptions should be made clear in the dialect. Do we say something like this in the definition of the MLIR operation? Alternatively, we can pass the Alloc or Release and then during lowering from the OpenMP dialect to LLVM IR, this can be handled appropriately.

flang/test/Lower/target_data.f90
106 ↗(On Diff #491418)
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
152

flang side : flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
mlir side : mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir

TIFitis updated this revision to Diff 491743.Jan 24 2023, 6:08 AM
TIFitis marked 2 inline comments as done.

Added new line to end of test file.

flang/lib/Lower/OpenMP.cpp
599

I've copied the comment from ClangCodeGen. OpenMPOffloadMappingFlags does not have enums for alloc and release and thus I can't really add them here.

Also, this is handled in the custom printer and parser, i.e, it still shows alloc or release appropriately in the fir and mlir. An absence of map_type implicitly assumes alloc or release based on the target directive.

ClangCodeGen reference: clang/lib/CodeGen/CGOpenMPRuntime.cpp::7065

flang/test/Lower/target_data.f90
106 ↗(On Diff #491418)

Sorry, I thought you were asking me to remove the line instead of adding one.

LGTM.

flang/lib/Lower/OpenMP.cpp
599

Nit: OK. Can you change the comment to refer to the OpenMP dialect oepration instead of the runtime?

flang/test/Lower/target_data.f90
11 ↗(On Diff #491743)

Nit: Why are all the values captured but not used? If you don't want to capture to compare later then you can probably just do {{.*}}.

This revision is now accepted and ready to land.Jan 25 2023, 1:34 PM