[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
kiranchandramohan accepted this revision. kiranchandramohan added a comment. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
kiranchandramohan added a comment. Thanks @peixin. I am going through the patch today and will accept or provide some comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
peixin added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
peixin added a comment. @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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
Meinersbur accepted this revision. Meinersbur added a comment. This revision is now accepted and ready to land. Accepting patch since no reaction from @fghanim CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
Meinersbur added a comment. Note that the `OpenMPIRBuilderTest.OrderedDirective` test is still crashing. Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2156-2157 + + Value *EntryBBTI = EntryBB->getTerminator(); + EXPECT_EQ(EntryBBTI, nullptr); + peixin wrote: > Meinersbur wrote: > > Consider emitting a terminator, call `finalize()` and `verifyModule`. > 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. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107430/new/ https://reviews.llvm.org/D107430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder
peixin added a comment.
@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.
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5323-5325
+Range = AssertSuccess(Actions.BuildBinOp(
+nullptr, {}, BO_Add, Range,
+Actions.ActOnIntegerConstant(SourceLocation(), 1).get()));
Meinersbur wrote:
> Haven't really understood why this is necessary, but this new version LGTM.
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, );
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.
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2154
+ Builder.restoreIP(
+ OMPBuilder.createOrdered(Builder, BodyGenCB, FiniCB, false));
+
Meinersbur wrote:
> Did you intend to pass IsThreads=true. Currently it is failing because no
> `__kmpc_ordered` is emitted.
Yes. Thanks for pointing out the problem.
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:2156-2157
+
+ Value *EntryBBTI = EntryBB->getTerminator();
+ EXPECT_EQ(EntryBBTI, nullptr);
+
Meinersbur wrote:
> Consider emitting a terminator, call `finalize()` and `verifyModule`.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107430/new/
https://reviews.llvm.org/D107430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
