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
863

final

866

Reformat the patch

868

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

906

Must return ArrayRef

clang/include/clang/AST/StmtOpenMP.h
348
  1. final
  2. Comments?
350

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
11902

Serialization for inner clauses

clang/test/OpenMP/metadirective_default.c
1–13

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
350

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
5800

Style: start names with upper case letter.

5816

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

5833

Doesn't this produce:

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

while we need:

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

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
2581–2583

Doc string missing, see above.

clang/include/clang/AST/OpenMPClause.h
866

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

868

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

888

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

903

Why does the clause have a directive kind?

clang/include/clang/Basic/DiagnosticSemaKinds.td
10133 ↗(On Diff #255575)

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
39 ↗(On Diff #255575)

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
95

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
5786

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

5789

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

5849

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?