This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP50] Add parallel master construct
ClosedPublic

Authored by cchen on Nov 26 2019, 9:26 AM.

Event Timeline

cchen created this revision.Nov 26 2019, 9:26 AM
Herald added a project: Restricted Project. · View Herald Transcript

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.

cchen updated this revision to Diff 231922.Dec 3 2019, 8:43 AM

Update based on feedback

ABataev added inline comments.Dec 3 2019, 9:30 AM
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.

cchen marked 2 inline comments as done.Dec 3 2019, 12:19 PM
cchen added inline comments.
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.

cchen marked an inline comment as done.Dec 3 2019, 12:28 PM
cchen added inline comments.
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.

cchen updated this revision to Diff 231977.Dec 3 2019, 1:30 PM

Fix tests and bugs based on feedback

ABataev added inline comments.Dec 3 2019, 1:36 PM
clang/test/OpenMP/parallel_master_codegen.cpp
2

A codegen test for if, proc_bind and allocate clauses?

cchen updated this revision to Diff 231995.Dec 3 2019, 2:40 PM

Add ignored codegen tests

ABataev added inline comments.Dec 4 2019, 7:02 AM
clang/include/clang/AST/StmtOpenMP.h
1875

No such param.

clang/test/OpenMP/parallel_master_codegen.cpp
364

Why do we need the outer parallel region here?

cchen updated this revision to Diff 232149.Dec 4 2019, 8:35 AM

Fix based on feedback

cchen marked 14 inline comments as done.Dec 4 2019, 8:53 AM
This revision is now accepted and ready to land.Dec 4 2019, 8:59 AM
cchen added a comment.Dec 4 2019, 9:30 AM

@ABataev, thanks for your time and patient for the review! Could you land this patch for me when you have free time? Thanks

This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Dec 4 2019, 2:51 PM

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.

ABataev reopened this revision.Dec 4 2019, 2:54 PM

Need to fix tests on Windows

This revision is now accepted and ready to land.Dec 4 2019, 2:54 PM
ABataev requested changes to this revision.Dec 4 2019, 2:55 PM
This revision now requires changes to proceed.Dec 4 2019, 2:55 PM
cchen added a comment.Dec 4 2019, 8:17 PM

How to see the test result on Windows? I've no windows machine to test the compiler.

cchen updated this revision to Diff 232357.Dec 5 2019, 8:39 AM

Fix codgen test for Windows

ABataev added inline comments.Dec 5 2019, 8:56 AM
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?

cchen updated this revision to Diff 232366.Dec 5 2019, 9:25 AM

Pass triple for all the test runs

ABataev added inline comments.Dec 5 2019, 9:39 AM
clang/test/OpenMP/parallel_master_codegen.cpp
68

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.

cchen updated this revision to Diff 232379.Dec 5 2019, 10:04 AM

Fix based on feedback

This revision is now accepted and ready to land.Dec 5 2019, 10:21 AM
cchen added a comment.Dec 5 2019, 10:26 AM

Can you commit it again for me? thanks!

This revision was automatically updated to reflect the committed changes.