This is an archive of the discontinued LLVM Phabricator instance.

OpenMP Metadirective with user defined condition
Needs ReviewPublic

Authored by alokmishra.besu on Mar 15 2020, 10:02 PM.

Details

Summary

This is a patch to implement dynamic extension to the user condition selector for OpenMP Metadirective.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2020, 10:02 PM

I think you forgot to add the tests to the commit :)

Adding test case

Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2020, 8:19 PM

Fixed build error and test files

ABataev added inline comments.
clang/include/clang/AST/OpenMPClause.h
870

final

873

Reformat the patch

875

Bad idea. If you need to store other clauses in this clause, use tail allocation.

913

Must return ArrayRef

clang/include/clang/AST/StmtOpenMP.h
365
  1. final
  2. Comments?
367

This must be a part of the tail-allocated data members, otherwise you won't be able to correctly handle list of children nodes

clang/lib/Serialization/ASTReader.cpp
12059

Serialization for inner clauses

clang/test/OpenMP/metadirective_default.c
1–13 ↗(On Diff #250830)

This is not a test, check other files in this directory

alokmishra.besu updated this revision to Diff 250966.EditedMar 17 2020, 7:29 PM
alokmishra.besu marked 6 inline comments as done.

Fixed formatting issues and replaced SmallVector with ArrayRef .

Working on the test cases.

clang/include/clang/AST/StmtOpenMP.h
367

This is not the associated statement. It is a newly generated statement which will only be used in CodeGen. I don't think we need to handle a list of children node for this one.

martong removed a subscriber: martong.Mar 18 2020, 7:51 AM
alokmishra.besu marked an inline comment as not done.

Updated the test files

Apologies for my delay.

Updated the test files

It seems there is only an AST dump test case. Where is the codegen tests (and others)?

jdoerfert added inline comments.Mar 30 2020, 7:53 AM
clang/lib/Sema/SemaOpenMP.cpp
6285

Style: start names with upper case letter.

6301

You need to use Diag here. See other uses in this file.

6318

Doesn't this produce:

if (when1) case1;
if (when2) case2;
...

while we need:

if (when1) case1;
else if (when2) case2;
...
12689

I think a simple if (DKind == ...) makes more sense here.

Is there an update on this?

alokmishra.besu marked 6 inline comments as done.Apr 6 2020, 8:28 PM

Updated the test cases.
Fixed an issue where correct code was not generated if directive variant was not provided in when/default clause

Are you planning to add support for the actual OpenMP selectors as well?
All the other traits, scoring, etc. I left a comment below that points you to the right place as we have almost all of that already ready to be reused.
On this note, I would recommend you take a look at the OpenMP 5.1 draft, it contains proper wording for a dynamic context extension, and we will need support in the declare variant as well. That means we need to allow dynamic expressions in OMPTraitInfo and similar places eventually anyway.

We need more tests:

  • verify all parser errors
  • verify semantic errors wrt the directives
  • verify the condition is captured properly, e.g., if it is in a parallel region
  • verify complex conditions work, e.g., globals, calls, constexpr expressions, template arguments, ...
  • verify an "empty" metadirective works
  • verify we exclude directives & conditions that should not be emitted

Fixed an issue where correct code was not generated if directive variant was not provided in when/default clause

We need a test for that too.

clang/include/clang-c/Index.h
2571–2575

Doc string missing, see above.

clang/include/clang/AST/OpenMPClause.h
873

This looks like it is tailored towards a single user condition. You should use a OMPTraitInfo object here, see how it is done for declare variant (which has the same kind of context selector): https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseOpenMP.cpp#L1388

875

This doesn't seem to be addressed but I'm actually unsure what the clauses of a when clause are?

895

The comment above refers to A and looks pretty much like it is copy and pasted from elsewhere.

910

Why does the clause have a directive kind?

clang/include/clang/Basic/DiagnosticSemaKinds.td
10337

Unsure about the bang above. We should probably have other errors of this kind with wording you can use. Also, the order of clauses is not important (for almost everything).

clang/test/OpenMP/metadirective_codegen.cpp
40

We need to check for the code of the outlined functions as well, from this it is not clear that there is a omp for in the second case. The formatting of these check lines is also problematic. Maybe start by using the llvm/utils/update_cc_test_checks.py script.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
34

This is not rebased, directives are now declared in the tablegen file:
llvm/include/llvm/Frontend/OpenMP/OMP.td

Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 8:54 PM
jdoerfert updated this revision to Diff 284961.Aug 11 2020, 9:07 PM

Update for @alokmishra.besu via arcanist as the webinterface doesn't allow patches over 2MB

Are you planning to add support for the actual OpenMP selectors as well?
All the other traits, scoring, etc. I left a comment below that points you to the right place as we have almost all of that already ready to be reused.
On this note, I would recommend you take a look at the OpenMP 5.1 draft, it contains proper wording for a dynamic context extension, and we will need support in the declare variant as well. That means we need to allow dynamic expressions in OMPTraitInfo and similar places eventually anyway.

The idea of this patch is to implement user condition for metadirective.
Anyways I will be adding support for other OpenMP selectors as well. I think doing that in a different patch will be a better idea, as there are lots of things to consider.
I do take this point and will update this patch to support future extension to other selectors.

We need more tests:

  • verify all parser errors
  • verify semantic errors wrt the directives
  • verify the condition is captured properly, e.g., if it is in a parallel region
  • verify complex conditions work, e.g., globals, calls, constexpr expressions, template arguments, ...
  • verify an "empty" metadirective works
  • verify we exclude directives & conditions that should not be emitted

Fixed an issue where correct code was not generated if directive variant was not provided in when/default clause

We need a test for that too.

Will update all these test cases soon.
.

Update for @alokmishra.besu via arcanist as the webinterface doesn't allow patches over 2MB

Currently trying to figure out how to use arcanist. :)

kkwli0 added a subscriber: kkwli0.Aug 31 2020, 2:54 PM

reverse ping?

Updated patch to make it extendable to more selectors
Implemented device_arch and implementation_vendor selectors as well

Some comments but I haven't gone through all of it. The tests are still missing.

clang/lib/Sema/SemaOpenMP.cpp
6290

i is not a name for whatever this is. Don't use auto if the type is not obvious, consider a reverse range adapter.

6293

NULL is a poor substitute for nullptr, use the latter, always.

6353

Do not reimplement parts of logic already existing. Use the same mechanism to find a match we use in other places. See ActOnOpenMPCall, and the llvm/Frontend/OpenMP/OMPContext.h.

Are you still working on this?

reverse ping. Do you have an estimate when you update this?