This is an archive of the discontinued LLVM Phabricator instance.

[mlir][openacc] Initial translation for DataOp to LLVM IR
ClosedPublic

Authored by clementval on Jun 15 2021, 8:11 AM.

Diff Detail

Event Timeline

clementval created this revision.Jun 15 2021, 8:11 AM
clementval requested review of this revision.Jun 15 2021, 8:11 AM
clementval added a project: Restricted Project.Jun 15 2021, 8:26 AM

Apologies for the delay.

I had a quick look through the patch. I feel like a lot of the code here can be put into the OpenACC/OpenMP IRBuilder. As you know the usual way the function for a construct is written in the OpenMP IRBuilder is like there is a function (createData) for creating the runtime calls for the construct and there is a bodygen callback that can fill in the body of the construct. The createData function in the OpenACC IRBuilder can create the runtime calls for beginMapperFunc, endMapperFunc. Is it possible to rewrite the code like this?

mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
270–310

Can these two functions be in the OpenACC IRBuilder?

371–377

This should be in the OpenACC IRBuilder I think.

381–417

This code can probably be the bodygen callback, which creates the mapping from mlir to llvm blocks and then converts them to LLVM IR.

420–428

This should be in the OpenACC IRBuilder I think.

Apologies for the delay.

I had a quick look through the patch. I feel like a lot of the code here can be put into the OpenACC/OpenMP IRBuilder. As you know the usual way the function for a construct is written in the OpenMP IRBuilder is like there is a function (createData) for creating the runtime calls for the construct and there is a bodygen callback that can fill in the body of the construct. The createData function in the OpenACC IRBuilder can create the runtime calls for beginMapperFunc, endMapperFunc. Is it possible to rewrite the code like this?

Thanks for the comments. At first I was thinking to put this in the OpenXXIRBuilder like other functions you mentioned. The only reason I did not is that even it would be in the IRBuilder, the clang code gen is using the CodeGenFunction to create the allocas through call to CreateMemTemp and this is likely not do-able in our case from MLIR. But if it is fine that the function the IRBuilder is only (partially at least) used from Flang then I 100% fine following your suggestion.

Apologies for the delay.

I had a quick look through the patch. I feel like a lot of the code here can be put into the OpenACC/OpenMP IRBuilder. As you know the usual way the function for a construct is written in the OpenMP IRBuilder is like there is a function (createData) for creating the runtime calls for the construct and there is a bodygen callback that can fill in the body of the construct. The createData function in the OpenACC IRBuilder can create the runtime calls for beginMapperFunc, endMapperFunc. Is it possible to rewrite the code like this?

Thanks for the comments. At first I was thinking to put this in the OpenXXIRBuilder like other functions you mentioned. The only reason I did not is that even it would be in the IRBuilder, the clang code gen is using the CodeGenFunction to create the allocas through call to CreateMemTemp and this is likely not do-able in our case from MLIR. But if it is fine that the function the IRBuilder is only (partially at least) used from Flang then I 100% fine following your suggestion.

We can discuss during today's call.

Move code to OpenMPIRBuilder

Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 11:51 AM

@kiranchandramohan I moved some of the code to the OpenMPIRBuilder. I left the region conversion in the translation for the moment. This region is a bit special since it could even be just inline between the two calls.

Thanks @clementval. I would request @jdoerfert, @Meinersbur, @fghanim to provide the feedback here.

MLIR part LGTM with comments addressed.

mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp
126–127

We usually pass SmallVectorImpl & that avoids the need to fix the number of stack elements. SmallVector has a fixed number even if this number is computed by template deduction.

280

No need for template here.

286–291

Please expand auto here. MLIR uses auto when the type is obvious from the RHS, e.g., in a dyn_cast, or when it is too long or impossible to spell out, e.g. iterators and lambdas.

384–386

It doesn't look like the insertion point changed between where currIP was created and here.

clementval marked 8 inline comments as done.

Address review comments

Addressed @ftynse comments.

@jdoerfert @Meinersbur Any comment on the OpenMPIRBuilder part?

jdoerfert added inline comments.Jul 21 2021, 8:40 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
39

If we have a struct with 3 alloca members with proper names this is not needed anymore.

2259

Also elsewhere.

2276

The types should all exist, Int8Ptr is a global in the omp namespace. If not, you can also add them to OMPKinds.def (top).

I don't think returning a SmallVector is great. I'd suggest a struct with 3 Alloca members with names or pass such a struct in and fill it.

Alloca IP has to be passed in from the caller. The entry point is not necessarily the right place, e.g., if this is called in a parallel region.

clementval marked 2 inline comments as done.

Address review comment

clementval marked an inline comment as done.Jul 22 2021, 11:09 AM

@jdoerfert Thanks for the review. I uploaded a new version.

jdoerfert accepted this revision.Jul 22 2021, 12:00 PM

OpenMPIRBuilder parts looks good

This revision is now accepted and ready to land.Jul 22 2021, 12:00 PM

Fix OpenMPIRBuilder unit test

This revision was landed with ongoing or failed builds.Jul 27 2021, 7:04 PM
This revision was automatically updated to reflect the committed changes.