Page MenuHomePhabricator

[OpenMP] Add initial support for `omp [begin/end] assumes`
ClosedPublic

Authored by jdoerfert on Nov 23 2020, 10:06 AM.

Details

Summary

The assumes directive is an OpenMP 5.1 feature that allows the user to
provide assumptions to the optimizer. Assumptions can refer to
directives (absent and contains clauses), expressions (holds
clause), or generic properties (no_openmp_routines, ext_ABCD, ...).

The assumes spelling is used for assumptions in the global scope while
assume is used for executable contexts with an associated structured
block.

This patch only implements the global spellings. While clauses with
arguments are "accepted" by the parser, they will simply be ignored for
now. The implementation lowers the assumptions directly to the
AssumptionAttr.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ABataev added inline comments.Nov 23 2020, 10:31 AM
clang/include/clang/Sema/Sema.h
10333

It may have side-effects with template instantiations. What if we have something like this:

#pragma omp begin assumes ...
template <typename T>
struct S {
...
}
#pragma omp end assumes

void bar() {
#pragma omp assumes ...
  {
    S<int> s;
    s.foo();
  }
}

?

struct S<int> will be instantiated in the second assume region context though I doubt this is the user intention.

10333–10336

const member functions.

jdoerfert added inline comments.Nov 23 2020, 1:30 PM
clang/include/clang/Sema/Sema.h
10333

I'm not sure what problem you expect here. Could you elaborate what you think should happen or should not?

ABataev added inline comments.Nov 23 2020, 1:33 PM
clang/include/clang/Sema/Sema.h
10333

May it lead to the wrong compiler assumptions and result in the incorrect codegen?

jdoerfert added inline comments.Nov 24 2020, 8:46 AM
clang/include/clang/Sema/Sema.h
10333

I'm still not following your example. What wrong assumptions and/or incorrect code generation do you expect? Could you provide an example with actual assumptions and then describe what the problem is please.

ABataev added inline comments.Nov 24 2020, 11:26 AM
clang/include/clang/Sema/Sema.h
10333

Say, you have something like this:

template <typename T>
struct S {
  int a;
  void foo() {
    #pragma omp parallel
    ...
  }
};

void bar() {
#pragma omp assumems no_openmp
  S<int> s; // S<int> is instantiated here and function S<int>::foo() gets no_openmp attribute.
  s.a = 0; // S<int>::foo is instantiated here but not used and the attribute no_openmp should not be propagated for this function.
#pragma omp end assumes

  S<int> b; // S<int> already instantiated and S<int>::foo is instantiated with attribute no_openmp
  b.foo(); // calls function S<int>::foo with no_openmp attribute.
}

What do you think?

jdoerfert added inline comments.Nov 24 2020, 12:45 PM
clang/include/clang/Sema/Sema.h
10333

Thanks for the extra information. I'll add a test based on this for sure, no matter what we come up with for now.

#pragma omp begin assumes no_openmp
  S<int> s; // S<int> is instantiated here and function S<int>::foo() gets no_openmp attribute.
  s.a = 0;
#pragma omp end assumes

Hm, I guess the question is:
Is S<int>::foo() a function defined in between the begin/end or not. If we say it is, the behavior right now seems consistent but the standard might need some clarifications. If we say it is not, we should not annotate it. Though, if it is not, where is it defined? I could avoid annotating template instantiations I guess. I might start with that. Update is coming.

ABataev added inline comments.Nov 24 2020, 12:50 PM
clang/include/clang/Sema/Sema.h
10333

Yep, this is what I'm asking about. I would check С++ standard, I believe we shall follow С++ rules here about the instantiation context.

jdoerfert updated this revision to Diff 307479.Nov 24 2020, 4:40 PM

Improve template handling. Do not annotate template instantiations with scoped assumptions for now.

jdoerfert added inline comments.Nov 25 2020, 11:07 AM
clang/include/clang/Sema/Sema.h
10333

I think the new solution, that is not to use scoped assumptions during template instantiations, is a conservative approximation of what people would assume and also matches the standard wrt. "defintition-declaration-seq". I included your example as a test.

WDYT?

Why don't yo want to try to implement the scheme similar to the declare target?

clang/include/clang/Sema/Sema.h
10333

const member function

10336

const member function

clang/lib/Parse/ParseOpenMP.cpp
1531

I think you will need just capture this by value here

1550–1555

Why not express it as clauses?

1560

S

1569

Idx

1569–1593

Better to outline it into function/lambda

clang/lib/Sema/SemaOpenMP.cpp
3210

What are you trying to do here? Scan all the decls in the current context and mark all the functions with the assumption attributes?

jdoerfert marked 8 inline comments as done.

Addressed comments

jdoerfert added a comment.EditedNov 25 2020, 12:56 PM

Why don't yo want to try to implement the scheme similar to the declare target?

Because it is not clear that the standard even says that right now. Also, what is the user expectation here.
The scheme now is conservative but consistent. I'd prefer to use something like that first before we clarify edge cases.

clang/lib/Parse/ParseOpenMP.cpp
1531

I've never done that. Could you explain why? Is it just an "optimization"? A simple test doesn't really show a difference https://godbolt.org/z/bfr47f

1550–1555

Various reasons actually:

  • It is a lot of boilerplate code, if we would auto-generate them from the tablegen definition the situation might be different.
  • There seems to be no immediate benefit.
  • We want to be in-sync with the generic assumption handling not develop two systems for the same thing.
1569–1593

Which part? Assuming you mean the 9 line conditional above, I'm not convinced. Even if I do outline it, 4 lines will have to stay here and I don't see any reuse.

clang/lib/Sema/SemaOpenMP.cpp
3210

Pretty much, yeah. Find all function declarations and add the attribute. I'll add a comment.

Why don't yo want to try to implement the scheme similar to the declare target?

Because it is not clear that the standard even says that right now. Also, what is the user expectation here.
The scheme now is conservative but consistent. I'd prefer to use something like that first before we clarify edge cases.

I don't think that it will affect the implementation significantly. Still, OpenMP standard must preserve visibility etc. rules of C/C++, so the changes should not be critical (I think).

clang/lib/Parse/ParseOpenMP.cpp
1531

Yes, this is just the optimization.It is not for the current implementation but for possible future changes. There is no difference for this but might be difference if you'll need to capture something else.

1550–1555

Just it is always to have and to maintain just one design rather than 2 or more. But it is up to you.

1569–1593

The part that searches over the table.

clang/lib/Sema/SemaOpenMP.cpp
3210

Does it mean that for this code

void foo();
 #pragma omp assume ...
void bar();
 #pragma omp end assume

foo() must have the same assumptions like bar()? It looks to me, currently we'll end up with the same attributes for both functions. But I'm not sure that this is correct to mark the function outside of the assumes region with the same attributes just like the function inside.

Why don't yo want to try to implement the scheme similar to the declare target?

Because it is not clear that the standard even says that right now. Also, what is the user expectation here.
The scheme now is conservative but consistent. I'd prefer to use something like that first before we clarify edge cases.

I don't think that it will affect the implementation significantly. Still, OpenMP standard must preserve visibility etc. rules of C/C++, so the changes should not be critical (I think).

We can add more support later, once it is clear how that will look like. As the person that wrote the OpenMP standard section, I'm trying to tell you it is not clear what the expected behavior is supposed to be. Not applying assumes is *always* correct. Applying them for some cases that is not easy to argue about, is at least confusing. If you feel we should add assumes in more cases, I'm happy to bring up the discussion in the OpenMP standards committee and look at a follow up patch.

clang/lib/Parse/ParseOpenMP.cpp
1569–1593

The part that searches over the table.

I don't follow. Where is a search? The part that builds a string switch? line 1569 - 1579?

clang/lib/Sema/SemaOpenMP.cpp
3210

Your example mixes the global omp assumes with the scoped begin/end omp assumes. The former has the effect to be added to prior declarations, the latter does not have this effect. This is how it is supposed to work:

void foo();  // no assumption
 #pragma omp begin assumes ext_XYZ
void bar();  // XYZ assumption
 #pragma omp end assumes
void foo();  // XYZ assumption
 #pragma omp assumes ext_XYZ
void bar();  // XYZ assumption
ABataev added inline comments.Nov 25 2020, 2:29 PM
clang/lib/Parse/ParseOpenMP.cpp
1569–1593

Yes.

clang/lib/Sema/SemaOpenMP.cpp
3210

Ah, I see. omp assumes looks weird. :( It may lead to conflicts
PS. What about lambdas? Could add a test that lambdas get annotations too?

3218

I would also add a check that the decl is valid here before checking it and applying the attributes.

3225
if (auto *SubCtx = dyn_cast<DeclContext>(SubDC)) {
  DeclContexts.push_back(cast<DeclContext>(SubCtx));
 continue;
}

?

jdoerfert marked 6 inline comments as done.Dec 1 2020, 4:53 PM
jdoerfert added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
3210

Ah, I see. omp assumes looks weird. :( It may lead to conflicts

The reason we have assumes and assume is to avoid conflicts. Let's hope it works.

PS. What about lambdas? Could add a test that lambdas get annotations too?

Done, assuming lambdas are function declarations. Otherwise we would need to exclude them from the scoped assumptions as we do for template instantiations now.

jdoerfert updated this revision to Diff 308817.Dec 1 2020, 4:54 PM
jdoerfert marked an inline comment as done.

Addressed comments, lambda handling and more

ABataev added inline comments.Dec 2 2020, 2:17 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1296–1297

There is err_omp_no_clause_for_directive defined in Sema. See Sema::ActOnOpenMPTargetDataDirective how to use it, better to reduce the number of diagnostics if possible

clang/lib/Sema/SemaLambda.cpp
999–1002

Are there any other function-like constructs that also should have markings? Coroutines maybe?

clang/lib/Sema/SemaOpenMP.cpp
3203

How this comment relates to the code?

3207

Add a message in for the assertion

3217

Maybe, better to use getLexicalParent? getParent returns semantic parent, while getLexicalParent - the lexical parent. Do you need to mark the declarations in the lexical context or in the semantic context?

jdoerfert updated this revision to Diff 309340.Dec 3 2020, 12:50 PM
jdoerfert marked 4 inline comments as done.

Addressed comments

clang/lib/Sema/SemaLambda.cpp
999–1002

Hm, can we postpone coroutines for now?

clang/lib/Sema/SemaOpenMP.cpp
3203

Leftover.

3207

Good catch!

3217

I go to the top, should not matter. I want all declarations so I start at the top most scope, then traverse everything. I go with lexical now.

ABataev added inline comments.Dec 3 2020, 1:31 PM
clang/lib/Sema/SemaOpenMP.cpp
3203–3206

Early exit from the function in case of error?

jdoerfert added inline comments.Dec 3 2020, 2:56 PM
clang/lib/Sema/SemaOpenMP.cpp
3203–3206

We can't always because we want to create an (empty) scope so we can diagnose the end assumes properly. I'll check in the non-scope case though.

jdoerfert updated this revision to Diff 309381.Dec 3 2020, 3:05 PM

Early exit

ABataev added inline comments.Dec 4 2020, 9:56 AM
clang/lib/Sema/SemaOpenMP.cpp
5940

You can also check if the decl is invalid here

5945–5946

Looks like FD==nullptr is impossible here. Or UTemplDecl->getTemplatedDecl() can return nullptr?

jdoerfert updated this revision to Diff 309584.Dec 4 2020, 11:07 AM
jdoerfert marked 2 inline comments as done.

Addressed comments

This revision is now accepted and ready to land.Dec 4 2020, 11:18 AM
This revision was landed with ongoing or failed builds.Dec 15 2020, 2:52 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
101

This is broken on gcc-5:

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101:1: error: could not convert '(const char*)"ext_"' from 'const char*' to 'llvm::StringLiteral'
 };
 ^

I guess I revert it for now :(

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
101

You don't happen to have a suggestion on how to fix this, do you? I don't have a gcc-5 lying around to try (I think).

In addition to the build error, it also broke check-clang everywhere as far as I can tell.
mac: http://45.33.8.238/macm1/292/step_6.txt
linux: http://45.33.8.238/linux/35399/step_7.txt
win: http://45.33.8.238/win/29934/step_7.txt

In addition to the build error, it also broke check-clang everywhere as far as I can tell.
mac: http://45.33.8.238/macm1/292/step_6.txt
linux: http://45.33.8.238/linux/35399/step_7.txt
win: http://45.33.8.238/win/29934/step_7.txt

I think I fixed 2 errors but I'm unsure about the linux one. I changed one test and I'll try it again as I find a workaround for gcc-5

In addition to the build error, it also broke check-clang everywhere as far as I can tell.
mac: http://45.33.8.238/macm1/292/step_6.txt
linux: http://45.33.8.238/linux/35399/step_7.txt
win: http://45.33.8.238/win/29934/step_7.txt

I think I fixed 2 errors but I'm unsure about the linux one. I changed one test and I'll try it again as I find a workaround for gcc-5

The same tests fail on linux and mac and win at these links:

Clang :: OpenMP/assumes_codegen.cpp (win, mac/m1)
Clang :: OpenMP/assumes_include_nvptx.cpp (linux, mac/x86, mac/m1)

(mac/x86 link: http://45.33.8.238/mac/25261/step_7.txt)