This is a patch to implement dynamic extension to the user condition selector for OpenMP Metadirective.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
| |
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 ↗ | (On Diff #250830) | This is not a test, check other files in this directory |
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. |
Apologies for my delay.
It seems there is only an AST dump test case. Where is the codegen tests (and others)?
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5801 | Style: start names with upper case letter. | |
5817 | You need to use Diag here. See other uses in this file. | |
5834 | Doesn't this produce: if (when1) case1; if (when2) case2; ... while we need: if (when1) case1; else if (when2) case2; ... | |
11987 | I think a simple if (DKind == ...) makes more sense here. |
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: |
Update for @alokmishra.besu via arcanist as the webinterface doesn't allow patches over 2MB
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.
.
Currently trying to figure out how to use arcanist. :)
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 | ||
---|---|---|
5787 | i is not a name for whatever this is. Don't use auto if the type is not obvious, consider a reverse range adapter. | |
5790 | NULL is a poor substitute for nullptr, use the latter, always. | |
5850 | 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. |
Doc string missing, see above.