This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Codegen for 'num_threads' clause in 'parallel' directive
ClosedPublic

Authored by ABataev on Sep 2 2014, 12:25 AM.

Details

Summary

This patch generates call to "kmpc_push_num_threads(ident_t *loc, kmp_int32 global_tid, kmp_int32 num_threads);" library function before calling "kmpc_fork_call" each time there is an associated "num_threads" clause in the "omp parallel" directive.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 13153.Sep 2 2014, 12:25 AM
ABataev retitled this revision from to [OPENMP] Codegen for 'num_threads' clause in 'parallel' directive.
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.Sep 22 2014, 1:08 AM
include/clang/AST/StmtOpenMP.h
131 ↗(On Diff #13153)

Make it "Gets a single" or "Gets the single"

132 ↗(On Diff #13153)

Also, make it clear that this asserts if there is more than one clause of the specified kind.

lib/AST/Stmt.cpp
1396 ↗(On Diff #13153)

"is at least" -> "are at least"
Extra space in "the specified"

lib/Sema/SemaOpenMP.cpp
2592 ↗(On Diff #13153)

Do you need all of these checks, or is value-dependent enough?

2613 ↗(On Diff #13153)

While it is true that omp_get/set_num_threads uses 'int' to represent the number of threads, I think this cast (trunc/ext) to int32_t should be done in CodeGen, not here (it is runtime specific -- the AST should retain the original type).

test/OpenMP/parallel_num_threads_codegen.cpp
5 ↗(On Diff #13153)

Don't need include-guards here.

hfinkel added inline comments.Sep 22 2014, 1:53 AM
test/OpenMP/parallel_num_threads_codegen.cpp
5 ↗(On Diff #13153)

n/m -- as you pointed out in the other review, these are needed because of the PCH testing.

ABataev added inline comments.Sep 22 2014, 6:07 AM
include/clang/AST/StmtOpenMP.h
131 ↗(On Diff #13153)

Ok

132 ↗(On Diff #13153)

Ok

lib/AST/Stmt.cpp
1396 ↗(On Diff #13153)

Fixed

lib/Sema/SemaOpenMP.cpp
2592 ↗(On Diff #13153)

I think we need all of them, because num_threads argument is not a constant, but an expression.

2613 ↗(On Diff #13153)

Ok, I'll fix it.

hfinkel added inline comments.Sep 22 2014, 7:42 AM
lib/Sema/SemaOpenMP.cpp
2592 ↗(On Diff #13153)

I'm not sure; I think this is worth testing. A similar issue same up with the patches for the assume_aligned attribute, and it turned out that only the ValueDependent check was needed. Note that sizeof(T), for example, is value-dependent, not type-dependent.

ABataev added inline comments.Sep 22 2014, 9:10 PM
lib/Sema/SemaOpenMP.cpp
2592 ↗(On Diff #13153)

Here are some excerpts from the header:
/ template<int Size, char (&Chars)[Size]> struct meta_string - value depend bound of Chars
/ template<typename T> void add(T x) { return x;} - x is type dependent - it can be used as num_threads(x)
/ template<typename T> void add() { return sizeof(sizeof(T()));} - return is instantiation-dependent - I think this check can be excluded
/ template<typename F, typename ...Types> void forward(const F &f, Types &&...args) { f(static_cast<Types&&>(args)...); } - call to f contains unexpanded parameter pack - num_threads(f(static_cast<Types&&>(args)...))

ABataev updated this revision to Diff 13967.Sep 22 2014, 9:23 PM

Update after review

hfinkel added inline comments.Sep 22 2014, 9:36 PM
lib/Sema/SemaOpenMP.cpp
2592 ↗(On Diff #13153)

Ah, you're right. num_threads is not necessarily a compile-time constant, so the situation is different.

hfinkel accepted this revision.Oct 7 2014, 7:11 AM
hfinkel edited edge metadata.

LGTM; thanks!

This revision is now accepted and ready to land.Oct 7 2014, 7:11 AM
ABataev closed this revision.Oct 13 2014, 1:34 AM
ABataev updated this revision to Diff 14787.

Closed by commit rL219599 (authored by @ABataev).