Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 41945 Build 42280: arc lint + arc unit
Event Timeline
Missing check for prohibited nesting of the barrier and worksharing directvies in the master regions. Plus, definitely no tests for this.
clang/include/clang/AST/StmtOpenMP.h | ||
---|---|---|
1857 | Why do we need this if cancel in parallel master is not supported? | |
clang/include/clang/Sema/Sema.h | ||
9564 | Not formatted | |
clang/lib/AST/StmtOpenMP.cpp | ||
335–349 | Why moved to a different place? | |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
2872–2884 | Make it static local, not a member function | |
clang/lib/Sema/SemaOpenMP.cpp | ||
10679 | parallel master is very similar to just parallel rather than with parallel master taskloop when it comes to the analysis of the clauses. | |
clang/test/OpenMP/parallel_master_codegen.cpp | ||
17–26 | Check that the region was outlined. Plus, codegen for some of the clauses are required. |
clang/include/clang/AST/StmtOpenMP.h | ||
---|---|---|
1879 | HasCancel param? | |
clang/lib/AST/StmtOpenMP.cpp | ||
560 | auto *Dir | |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
2933 | Do we need copyins here at all? We have only master thread active here in this construct. | |
2950 | Seems to me, missed this code at the end: emitPostUpdateForReductionClause(*this, S, [](CodeGenFunction &) { return nullptr; }); | |
clang/lib/Sema/SemaOpenMP.cpp | ||
4015 | This is wrong, I believe. you don't have pure master region here, it is parallel master and it must be allowed to use it in the worksharing and tasking regions. | |
clang/test/OpenMP/nesting_of_regions.cpp | ||
2999 | Are simds allowed in master? | |
3024 | Is this allowed by the standard? | |
clang/test/OpenMP/parallel_master_ast_print.cpp | ||
16–22 | Add printing of all supported clauses, take a look at other AST printing tests. |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
---|---|---|
2933 | I think we still need to emit copyins since the assignment to the threadprivate copies of the other threads must be observable after the parallel region. | |
clang/test/OpenMP/nesting_of_regions.cpp | ||
2999 | I didn't see restriction about simds in master and there is also a test under "MASTER DIRECTIVE" allows simd directive inside master construct. |
clang/test/OpenMP/nesting_of_regions.cpp | ||
---|---|---|
3024 | I didn't see any restriction about master directive inside master directive. Also, nesting_of_regions also have tests for nested master construct. |
clang/test/OpenMP/parallel_master_codegen.cpp | ||
---|---|---|
2 | A codegen test for if, proc_bind and allocate clauses? |
@ABataev, thanks for your time and patient for the review! Could you land this patch for me when you have free time? Thanks
One of the new tests doesn't pass on Windows, so I reverted this in rG33f6d465d790ac5c9b949e6bc05127d356212079. I made an attempt to fix the checks, but it wasn't trivial.
clang/test/OpenMP/parallel_master_codegen.cpp | ||
---|---|---|
145 | I think it would be better to pass the triple in all the test runs. Some of your test runs do not include triple and are executed using the default (host-based) triple. Did you try to run the tests with the 32bit triples, for example? |
clang/test/OpenMP/parallel_master_codegen.cpp | ||
---|---|---|
67 | Not the best idea to use %.global_tid. and %.bound_tid. or similar швы directly in the test, they may be different on different builds. Better to use regular expressions everywhere in the test. |
Why do we need this if cancel in parallel master is not supported?