This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpenMP] omp.sections and omp.section lowering to LLVM IR
ClosedPublic

Authored by shraiysh on Dec 3 2021, 1:31 AM.

Details

Summary

This patch adds lowering from omp.sections and omp.section (simple lowering along with the nowait clause) to LLVM IR.
Tests for the same are also added.

Diff Detail

Event Timeline

shraiysh created this revision.Dec 3 2021, 1:31 AM
shraiysh requested review of this revision.Dec 3 2021, 1:31 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
ftynse accepted this revision.Dec 3 2021, 3:50 AM

MLIR part LGTM with comments addressed. Please have the OpenMP part reviewed by @jdoerfert or @Meinersbur, or factor it out into a separate patch.

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

You probably want to return failure() and stop the conversion process.

574

Nit: please expand auto here. Also, let's not perpetuate the very outdated inst/instruction mlir terminology.

579

Nit: expand auto here too please. MLIR prefers explicit types unless they are too long (iterators) or impossible to spell (lambdas), or if the type is obvious from the RHS (essentially if the RHS ends with a cast).

580

Nit: lowerCamelCase in MLIR please, here and below.

This revision is now accepted and ready to land.Dec 3 2021, 3:50 AM
shraiysh updated this revision to Diff 391874.Dec 4 2021, 1:09 PM

Thanks for the review @ftynse. Addressed comments. Waiting for review on OpenMP Part.

shraiysh marked 4 inline comments as done.Dec 4 2021, 1:11 PM

Thanks for the patch. LGTM

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

Thanks for the patch @shraiysh. I have a question. lastprivate and allocate clauses are supported in PFT to MLIR lowering. Would there be any cause for any problem when we integrate the entire toolchain for sections construct?

Thanks for the review @NimishMishra.

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

As long as they are not supported in atleast one part of the toolchain, we cannot actually support those clauses and giving an error is better than generating a binary unexpected behavior. So, till there is support for these clauses here flang will not be able to support them.

shraiysh marked an inline comment as done.Dec 5 2021, 12:29 AM

LGTM. I had a look and also compared the behaviour with the clang compiler (without IR builder) and it matches.

Since the change in the OpenMP IRBuilder is minor and to make progress I am approving this although @ftynse specifically asked for a review from @Meinersbur or @jdoerfert. @shraiysh if there are post-commit reviews, please address them.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
592–593

Nit: Can you expand the comment to say that this situation is only possible if there is only a terminator operation inside the sections operation? May be also assert for that situation.

shraiysh updated this revision to Diff 394214.EditedDec 14 2021, 5:13 AM

Rebase with main. Addressed comment. Sure @kiranchandramohan. Will address if there are any post-commit reviews.

shraiysh marked an inline comment as done.Dec 14 2021, 5:14 AM
shraiysh updated this revision to Diff 394294.Dec 14 2021, 9:48 AM

Rebase with main. (Unrelated build failure)

shraiysh updated this revision to Diff 394465.Dec 14 2021, 9:42 PM

Rebase with main

Since the change in the OpenMP IRBuilder is minor and to make progress I am approving this although @ftynse specifically asked for a review from @Meinersbur or @jdoerfert.

Anyone with sufficient knowledge of OpenMPIRBuilder really. I just know those two have it.

@ftynse @kiranchandramohan there are unrelated build failures (and this is happening for other patches too). Looks like there is an issue with the build job. Should I land this change without waiting for the build jobs to be rectified?

@ftynse @kiranchandramohan there are unrelated build failures (and this is happening for other patches too). Looks like there is an issue with the build job. Should I land this change without waiting for the build jobs to be rectified?

Please go ahead @shraiysh .

This revision was landed with ongoing or failed builds.Dec 15 2021, 2:11 AM
This revision was automatically updated to reflect the committed changes.