This is an archive of the discontinued LLVM Phabricator instance.

[OMPIRBuilder] Add ordered directive to OMPIRBuilder
ClosedPublic

Authored by peixin on Aug 4 2021, 12:54 AM.

Details

Summary

Add support for ordered directive in the OpenMPIRBuilder.

This patch also modidies clang to use the ordered directive when the
option -fopenmp-enable-irbuilder is enabled.

Also fix one ICE when parsing one canonical for loop with the relational
operator LE or GE in openmp region by replacing unary increment
operation of the expression of the variable "Expr A" minus the variable
"Expr B" (++(Expr A - Expr B)) with binary addition operation of the
experssion of the variable "Expr A" minus the variable "Expr B" and the
expression with constant value "1" (Expr A - Expr B + "1").

Diff Detail

Event Timeline

peixin created this revision.Aug 4 2021, 12:54 AM
peixin requested review of this revision.Aug 4 2021, 12:54 AM

Thanks @peixin for the patch. Could you remove the OpenMP 1.0 target from the description? That is a target for the Flang team and since this patch touches clang as well, it might be confusing. I believe this is modelling the ordered construct with a region. Could simd be another option to CreateOrdered or does it need more changes? Also, could you clarify how the standalone ordered construct (with depend) will be handled? Will it be an extension to CreateOrdered or will be a completely separate function?

peixin edited the summary of this revision. (Show Details)Aug 4 2021, 5:45 AM
peixin added a comment.EditedAug 4 2021, 6:15 AM

@kiranchandramohan Thanks for the review. Removed the OpenMP 1.0 target from the description.

The simd clause does need more changes and I think it also depends on simd directive in the OpenMPIRBuilder. According to the CHECK5 in the test case ordered_codegen.cpp, it does not create kmp_ordered runtime functions call with the simd clause.

I have not investigated too much about the standalone ordered construct. To be honest, I am not sure if supporting simd and depend clauses in CreateOrdered is better or not. My current plan is to look into the Flang MLIR Op Def and LLVM IR to understand how the IRBuilder is used. After finishing the end-to-end implementation of ordered construct without simd and depend clauses, I will come back to try to implement the IRBuilder of ordered directive with simd and depend clauses. At that time, Arnamoy should also have time to implement the IRBuilder for simd directive.

Meinersbur added inline comments.Aug 4 2021, 2:09 PM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
708

Could you commit these typo fixes separately? That is, just push it to main, no Phabricator review required.

721

Could you commit these typo fixes separately? That is, just push it to main, no Phabricator review required.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2125

Could you give this alloca register a name?

peixin updated this revision to Diff 364389.Aug 5 2021, 2:22 AM

@Meinersbur Thanks for the review.

Removed the typo fixes and gave the alloca register one name.

Also found some typos of invalid case style for variable 'cur' in llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp and will fix them when pushing this patch to main branch.

peixin updated this revision to Diff 365170.Aug 9 2021, 5:59 AM

Revise the implementation by not generating "kmp_ordered" runtime functions when -fopenmp-enable-irbuilder is not enabled, which is consistent with that not using OpenMPIRBuilder.

While manually editing ordered_codegen.cpp gives nicer results, re-running update_cc_test_checks.py would be less work. Your manual changes would be also be dropped the next time somebody runs update_cc_test_checks.py and commits.

The code seems derived from @fghanim single/master/etc implementation, would be nice if they could have a look.

The non-OMPBuilder code emits an outlined __captured_stmt, but not with OMPBuilder enabled. I assume this is due to the missing finatlize call.

clang/lib/CodeGen/CGStmtOpenMP.cpp
5335

Also, out and assert/llvm_unreachable here?

clang/lib/Sema/SemaOpenMP.cpp
5323–5326

Haven't really understood why this is necessary, but this new version LGTM.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2152

Did you intend to pass IsThreads=true. Currently it is failing because no __kmpc_ordered is emitted.

2154–2155

Consider emitting a terminator, call finalize() and verifyModule.

peixin updated this revision to Diff 366879.Aug 17 2021, 6:27 AM
peixin retitled this revision from [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder to [OMPIRBuilder] Add ordered directive to OMPIRBuilder.
peixin edited the summary of this revision. (Show Details)

Support ordered directive in OMPIRBuilder to OMP 5.0 standard. That is, support ordered directive without clause, with threads, simd or depend clause.

Without clause specified, it behaves as if the threads clause is specified. With the threads clause specified, emit the entry and exit function calls before and after the inlined statements inside ordered region.

To understand simd clause specified, we need to look at the history commits for ordered codegen in clang Non-OMPIRBuilder code. The ordered simd codegen was implemented by Alexey Bataev to generate the outlined function for the statements inside ordered region (llvm-svn: 248772). Joseph Huber adds the always inline attribute to the outlined function when the optimization option is enabled since it is safe and it could potentially lead to performance degredation due to
inflated register counts in the parallel region (https://reviews.llvm.org/D106799). Alexey Batave supports the exception throw in llvm-svn 210098. According to OMP 5.0 standard, for C++ code, a throw executed inside a ordered region must cause execution to resume within the smae ordered region, and the same thread that threw the exception must catch it. I keep the Non-OMPIRBuilder code design and add one more EmitCaptureStmt function in OMPBuilderCBHelpers to support the exception throw for ordered simd.

With depend clause specified, the works can be split into three steps, analyzing the expressions of depend vector such as "(i, j-1) of depend(sink: i, j-1)", store the analyzed result values into depend vector, and generate runtime function call with the depend vector base address argument. I leave the analyzing experssions work in clang codegen since it is a little bit complicated to do it in OMPIRBuilder. Instead, several lines of code are enough in clang codegen. The code for other two steps are refactored in OMPIRBuilder.

@Meinersbur Thanks for your review and suggestions.

While manually editing ordered_codegen.cpp gives nicer results, re-running update_cc_test_checks.py would be less work. Your manual changes would be also be dropped the next time somebody runs update_cc_test_checks.py and commits.

Thanks for the notice. I followed the other OMPIRBuilder PRs in order to make review work easily. It should do not matter my manual changes are dropped the next time.

The code seems derived from @fghanim single/master/etc implementation, would be nice if they could have a look.

Yes. He is in the reviewers list.

The non-OMPBuilder code emits an outlined __captured_stmt, but not with OMPBuilder enabled. I assume this is due to the missing finatlize call.

Sorry about the misguide. It is not due to missing finalize call. My last version of patch only implements the code for ordered threads. So I use it for ordered simd test. The non-OMPIRBuilder code emits the outlined function call for ordered simd, while emits the inlined statements for ordered threads. Now the non-OMPIRBuilder code and OMPIRBuilder code generate the similar IRs.

clang/lib/Sema/SemaOpenMP.cpp
5323–5326

This is the problem of doing operation ++(Expr A - Expr B), which should be replaced with (Expr A - Expr B + "1"). To understand why it is not supportedin clang sema, you may need to look at the function stack calls, which I listed as follows:

SemaOpenMP.cpp: BuildUnaryOp(…Expr…)->
SemaExpr.cpp: BuildUnaryOp()->CreateBuiltinUnaryOp()->CheckIncrementDecrementOperand() ->CheckForModifiableLvalue() {
  Expr::isModifiableLvalueResult IsLV = E->isModifiableLvalue(S.Context, &Loc);
  case Expr::MLV_ClassTemporary:
      DiagID = diag::err_typecheck_expression_not_modifiable_lvalue;
}

The root reason is that the temporary expression (Expr A - Expr B) is not modifiable LValue. I revised the commit message. Please take a look at it and check if it is ok for you.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2152

Yes. Thanks for pointing out the problem.

2154–2155

Why do you want me to emit the terminator? If it is because you think the outlined captured function is not generated due to finalize call, there is no need. Discussed above. Sorry about the misguide.

Note that the OpenMPIRBuilderTest.OrderedDirective test is still crashing.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2154–2155

Without terminator, verifyModule will complain about it being missing. verifyModule should be called to ensure that the emitted IR is well-formed. Anyway, you seem to have added it in the last diff update.

peixin updated this revision to Diff 367412.Aug 19 2021, 12:41 AM

@Meinersbur Thanks for your reply. Add emitting the terminator and finalize and verfyModule calls.

Meinersbur accepted this revision.Aug 31 2021, 9:33 AM

Accepting patch since no reaction from @fghanim

This revision is now accepted and ready to land.Aug 31 2021, 9:33 AM
peixin added a comment.Sep 2 2021, 8:53 AM

@Meinersbur Thanks a lot for your review and accepting this patch.
BTW, I finished the implementation of the codegen of ordered construct for LLVM Flang and the OpenMP IRBuilder of ordered construct in this patch also works well for LLVM Flang. Is it OK to land this patch now or does it need more review?

peixin added a comment.Sep 2 2021, 8:56 AM

Here is the PR for codegen of ordered construct for LLVM Flang: https://github.com/flang-compiler/f18-llvm-project/pull/1027.
I create the PR on fir-dev branch since the test of lowering parse-tree to MLIR for LLVM Flang is still not supported in upstream llvm-project.

Thanks @peixin. I am going through the patch today and will accept or provide some comments.

This revision was automatically updated to reflect the committed changes.