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, &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.
================
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
