Page MenuHomePhabricator

[OpenMP] Implement '#pragma omp tile'
Needs ReviewPublic

Authored by Meinersbur on Mar 17 2020, 8:33 PM.

Details

Summary

The tile directive is in OpenMP's Technical Report 8 and foreseeably will be part of the upcoming OpenMP 5.1 standard.

This implementation is based on an AST transformation providing a de-sugared loop nest. This makes it simple to forward the de-sugared transformation to loop associated directives taking the tiled loops. In contrast to other loop associated directives, the OMPTileDirective does not use CapturedStmts. Letting loop associated directives consume loops from different capture context would be difficult.

A significant amount of code generation logic is taking place in the Sema class. Eventually, I would prefer if these would move into the CodeGen component such that we could make use of the OpenMPIRBuilder, together with flang. Only expressions converting between the language's iteration variable and the logical iteration space need to take place in the semantic analyzer: Getting the of iterations (e.g. the overload resolution of std::distance) and converting the logical iteration number to the iteration variable (e.g. overload resolution of iteration + .omp.iv). In clang, only CXXForRangeStmt is also represented by its de-sugared components. However, OpenMP loop are not defined as syntatic sugar. Starting with an AST-based approach allows us to gradually move generated AST statements into CodeGen, instead all at once.

I would also like to refactor checkOpenMPLoop into its functionalities in a follow-up. In this patch it is used twice. Once for checking proper nesting and emitting diagnostics, and additionally for deriving the logical iteration space per-loop (instead of for the loop nest).

Diff Detail

Event Timeline

Meinersbur created this revision.Mar 17 2020, 8:33 PM
Meinersbur edited the summary of this revision. (Show Details)
  • Rebase
  • Remove unrelated change
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 11:14 PM
  • Rebase after clang-format of cindex
JonChesterfield added a comment.EditedMar 25 2020, 9:53 AM

edit: actually you've already done the clang-format on trunk as I hoped, phabricator mislead me. Apologies for the noise

edit: actually you've already done the clang-format on trunk as I hoped, phabricator mislead me. Apologies for the noise

The previous diff was against a branch with only the whitespace changes to get rid of the noise in the diff. It seems that the pre-merge check instead compared formatting to the master (probably Arcanist/Phabricator looked for the common ancestor of origin/master and my git branch). This let to weird formatting warnings that weren't even changes highlighted in this diff. They are gone now with clang-formatted origin/master. Fotunately.

jdenny added a subscriber: jdenny.Mar 31 2020, 12:49 PM
ABataev added inline comments.Apr 14 2020, 1:42 PM
clang/include/clang/AST/StmtOpenMP.h
4781–4784

Not sure that this is a good idea to treat this directive as the executable directive. To me, it looks like kind of AttributedStmt. Maybe better to introduce some kind of a new base node for this and similar constructs, which does not own the loop but is its kind of attribute-like entity?
Also, can we have something like:

#pragma omp simd
#pragma omp tile ...
for(...) ;

Thoughts?

Meinersbur marked an inline comment as done.Apr 14 2020, 5:23 PM
Meinersbur added inline comments.
clang/include/clang/AST/StmtOpenMP.h
4781–4784

While not executed at runtime, syntactically it is parsed like a executable (loop-associated) directive. IMHO it does 'own' the loop, but produces another one for to be owned(/associated) by a different directive, as in your tile/simd example, which should already work. Allowing this was the motivation to do the transformation on the AST-level for now.

Herald added a project: Restricted Project. · View Herald Transcript
DavidTruby resigned from this revision.Mon, May 4, 10:24 AM