OpenMP 5.0 metadirective

[alokmishra.besu]

Summary

This patch supports OpenMP 5.0 metadirective features.
It is implemented keeping the OpenMP 5.1 features like dynamic user condition in mind.

A new function, getBestWhenMatchForContext, is defined in llvm/Frontend/OpenMP/OMPContext.h

Currently this function return the index of the when clause with the highest score from the ones applicable in the Context.
But this function is declared with an array which can be used in OpenMP 5.1 implementation to select all the valid when clauses which can be resolved in runtime. Currently this array is set to null by default and its implementation is left for future.

[jdoerfert]

This looks close to an OpenMP 5.0 implementation. I left comments inlined.

We need tests that show how non-selected alternatives *do not* impact the program. As an example, a template instantiation inside of a non-selected alternative is not actually performed.

We also need test with ill-formed metadirectives.

clang/include/clang/Basic/DiagnosticSemaKinds.td

10529

no !. The default clause doesn't need to be in the end either.

clang/include/clang/Serialization/ASTBitCodes.h

1952

Unrelated.

clang/lib/CodeGen/CGOpenMPRuntime.cpp

10211

Can you explain this, this seems odd to me.

clang/lib/Parse/ParseOpenMP.cpp

2313

Usually you would check the return value in case we later actually propagate errors while parsing the context selector.

2320

If we give up it should be an error, I think. If we issue a warning we just pretend the colon was there afterwards.

2331

We have balanced trackers for this that also deal with the fact that there might be a ) missing. This code will probably crash.

2343

Add a TODO that nullptr should be replaced as per the use in Sema::ActOnOpenMPCall.

2364

Use a BalancedDelimiterTracker.

2381

If you warn and continue above you need to check for : here again.

2387

What is this token? We have skipped this part before so we need to validate it is what we expect it to be.

2391

Should we not go back to the original code handling "directives" instead? This looks like it is copied here.

2444

Same as below, change the order. Also, the "skipping" part is always the same, put it in a helper function or lambda.

2451

Move the smaller case first and use an early exit. That will reduce the indention of the larger case by 1.

clang/test/OpenMP/metadirective_construct.cpp

12 ↗

(On Diff #306974)

Since when does the construct trait work? I'm confused.

clang/test/OpenMP/metadirective_empty.cpp

2

no -fopenmp-targets please.

clang/test/OpenMP/metadirective_implementation.cpp

1 ↗

(On Diff #306974)

Can we run this for all configurations of the metadirective so we can actually see it will pick the right one, not the first?

llvm/include/llvm/Frontend/OpenMP/OMPContext.h

197 ↗

(On Diff #306974)

Leftover.

llvm/lib/Frontend/OpenMP/OMPContext.cpp

337 ↗

(On Diff #306974)

This looks like a clone of getBestVariantMatchForContext with an extra unused argument.

400 ↗

(On Diff #306974)

Leftover.

[alokmishra.besu]

I have replied to the comments and will update the code accordingly. Some of the codes are intentionally left to be update in 5.1 implementation. Will add TODO comments there.
I will revisit the implementation of getBestWhenMatchForContext and also add more test cases over the weekend and submit new code by next week.

clang/include/clang/Basic/DiagnosticSemaKinds.td

10529

OK. Will update accordingly.

clang/include/clang/Serialization/ASTBitCodes.h

1952

Only STMT_OMP_META_DIRECTIVE was added. Rest was formatted by git clang-format

clang/lib/CodeGen/CGOpenMPRuntime.cpp

10211

Good catch. This was an experimental code for 5.1. Got committed by mistake. Will update.

clang/lib/Parse/ParseOpenMP.cpp

2313

OK. Will update accordingly.

2320

OK. Will update accordingly.

2331

Come to think of it, in case of a missing ')' this code might end up in an infinite loop. Will update accordingly.

2343

Will do

2381

Will update accordingly.

2391

Unfortunately we cannot go to the original code handling since the original code handling assumes that the directive always ends with annot_pragma_openmp_end, while here it will always end with ')'.
In specification 5.0, since we are choosing only 1 directive, the body of the while block remains the same as the original code. Only the condition of the while block changes. In specification 5.1, we will need to generate code for dynamic handling and even the body will differ as we might need to generate AST node for multiple directives. It is best if we handle this code here for easier handling of 5.1 code, than in the original code space.
I will add a TODO comment here.

2444

OK

2451

OK

clang/test/OpenMP/metadirective_empty.cpp

2

OK

clang/test/OpenMP/metadirective_implementation.cpp

1 ↗

(On Diff #306974)

Good catch. I will go through the implementation of getBestWhenMatchForContext to check that.

llvm/include/llvm/Frontend/OpenMP/OMPContext.h

197 ↗

(On Diff #306974)

Will remove.

llvm/lib/Frontend/OpenMP/OMPContext.cpp

337 ↗

(On Diff #306974)

I intended to keep it similar for 5.0 to be updated in 5.1 code. But anyways it seems to be giving wrong result. Will go through this again.

400 ↗

(On Diff #306974)

Will remove.

[alokmishra.besu]

Updated version of metadirective supporting OpenMP 5.0 features

[ABataev]

clang/include/clang/AST/StmtOpenMP.h

373

I think, metadirective should be a kind of a container for different sub-directives. The problem is that that subdirectives could be completely different, they may capture different variables, using different capture kind (by value or by reference) etc.So, you need to generate each possible sub-directive independently and store them in the meta directive node. Otherwise you won't be able to generate the code correctly.

[alokmishra.besu]

clang/include/clang/AST/StmtOpenMP.h

373

In OpenMP 5.0, we do not need to generate every sub-directive. Rather we need to select one (or none) directive and replace metadirective with it. So this is not needed.
Yes with future specifications we will need to include a list of all valid directives which need to be resolved at runtime. That is when we will need to generate and store multiple sub-directives inside the OMPMetaDirective class.

[ABataev]

clang/include/clang/AST/StmtOpenMP.h

373

I think you still need to do it even for 5.0. I don't mean y need to emit the code for every sub-directive, but to generate them in the AST. Even in the example above, when(user={condition(N>10)} is used in the template and N is a template argument, the chosen directive can be selected on the instantiation and, thus, the original templated directive has to contain all possible sub-directives.

[jdoerfert]

clang/include/clang/Serialization/ASTBitCodes.h

1952

Rest was formatted by git clang-format

Please don't format unrelated code.

clang/lib/Parse/ParseOpenMP.cpp

2391

Unfortunately we cannot go to the original code handling since the original code handling assumes that the directive always ends with annot_pragma_openmp_end, while here it will always end with ')'.

Let's add a flag to the original handling to make this possible then. Copying it is going to create more long term problems.

llvm/lib/Frontend/OpenMP/OMPContext.cpp

337 ↗

(On Diff #306974)

Use the getBestVariantMatchForContext interface now and for the 5.1 semantics we introduce the OrderedMatch. That can also be used in the call site declare variant handling which currently calls getBestVariantMatchForContext multiple times.

[alokmishra.besu]

clang/include/clang/Serialization/ASTBitCodes.h

1952

Removed.

clang/lib/Parse/ParseOpenMP.cpp

2391

Done.

[dreachem]

In D91944#2414364, @jdoerfert wrote:

This looks close to an OpenMP 5.0 implementation. I left comments inlined.

We need tests that show how non-selected alternatives *do not* impact the program. As an example, a template instantiation inside of a non-selected alternative is not actually performed.

We also need test with ill-formed metadirectives.

@jdoerfert I'm still trying to understand this thing regarding template instantiations. The spec says that a directive variant associated with a when clause can only affect the program if it is a dynamic replacement candidate. I had assumed this is referring to the runtime behavior of the program, and not (for instance) whether a compiler error is emitted due to a failing static assert.

Also, in order to determine what are the dynamic replacement candidates, and their order, all specified score expressions would need to be evaluated. Then, you'd need to evaluate the static context selectors, in decreasing order of their score, to find which one is the last dynamic replacement candidate. So I think template instantiations could be possible for those expressions, even if the corresponding variants aren't selected?

clang/include/clang/AST/StmtOpenMP.h

373

r

[jdoerfert]

In D91944#2457744, @dreachem wrote:

In D91944#2414364, @jdoerfert wrote:

This looks close to an OpenMP 5.0 implementation. I left comments inlined.

We need tests that show how non-selected alternatives *do not* impact the program. As an example, a template instantiation inside of a non-selected alternative is not actually performed.

We also need test with ill-formed metadirectives.

@jdoerfert I'm still trying to understand this thing regarding template instantiations.

The problem is this patch can only resolve to a single directive during parsing. Take

template<bool b>
void foo() {
  #pragma omp metadirective when(user={condition(b)}) ...
}

which is not resolvable at parsing time but it is a valid OpenMP 5.0 use of the metadirective
that needs to resolve at compile time.

The spec says that a directive variant associated with a when clause can only affect the program if it is a dynamic replacement candidate. I had assumed this is referring to the runtime behavior of the program, and not (for instance) whether a compiler error is emitted due to a failing static assert.

I always assume(d) that static_assert is something "affecting the program". It doesn't matter though because if you instantiate all the clauses eagerly you change things for the runtime as well, e.g., you can cause different template instances to be selected later on depending on earlier instantiations. So it's not "just" static_assert.

Also, in order to determine what are the dynamic replacement candidates, and their order, all specified score expressions would need to be evaluated. Then, you'd need to evaluate the static context selectors, in decreasing order of their score, to find which one is the last dynamic replacement candidate. So I think template instantiations could be possible for those expressions, even if the corresponding variants aren't selected?

Yes, we will always evaluate selectors and scores. The key is that we won't evaluate the directive variant, which is neither a selector nor a score.

I hope this makes sense.

[jdoerfert]

As discussed in the OpenMP in LLVM call on December 9th, we should go ahead with this patch and add support for the missing features, e.g., dependent selectors, afterwards. LGTM

[jdoerfert]

(Please make sure all clang and clang-tidy tests pass properly so we can merge the patch)

[ABataev]

clang/lib/Sema/TreeTransform.h

8385–8390

It would not work correctly, I would replace it with just error emission that instantiation is not supported yet.

[alokmishra.besu]

All clang test and clang-tidy pass. Ran clang-format on all code, except clang/include/clang/Serialization/ASTBitCodes.h which was formatting unrelated code as well.

[alokmishra.besu]

Emitting error instead of instantiation in clang/lib/Sema/TreeTransform.h. Added a TODO for future.

[alokmishra.besu]

Rebased with current branch to avoid patch failure

[cchen]

@alokmishra.besu, I'm trying to play/learn with your patch so I downloaded the diff and apply it to the master branch, however, I'm getting error messages everywhere if llvm::omp::OMPD_metadirective appears:

llvm-project/clang/include/clang/AST/StmtOpenMP.h:384:43: error: no member named 'OMPD_metadirective' in namespace 'llvm::omp'
                               llvm::omp::OMPD_metadirective, SourceLocation(),
                               ~~~~~~~~~~~^

Do you know why this is happening? (I've also tried clean build it but still get the error msg) Thanks!

Edit: I've been successfully compiled it after changing a computer, thanks.

[jdoerfert]

Are the test all passing?