[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

Peixin Qiao via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 17 06:46:12 PDT 2021


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



More information about the cfe-commits mailing list