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

10493

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

2186

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

2193

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

2204

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

2216

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

2237

Use a BalancedDelimiterTracker.

2254

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

2260

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

2264

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

2317

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

2324

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

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

clang/test/OpenMP/metadirective_empty.cpp

1

no -fopenmp-targets please.

clang/test/OpenMP/metadirective_implementation.cpp

1

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

Leftover.

llvm/lib/Frontend/OpenMP/OMPContext.cpp

337

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

400

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

10493

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

2186

OK. Will update accordingly.

2193

OK. Will update accordingly.

2204

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

2216

Will do

2254

Will update accordingly.

2264

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.

2317

OK

2324

OK

clang/test/OpenMP/metadirective_empty.cpp

1

OK

clang/test/OpenMP/metadirective_implementation.cpp

1

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

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

197

Will remove.

llvm/lib/Frontend/OpenMP/OMPContext.cpp

337

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

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

2264

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

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

2264

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

8383–8388

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?

[protze.joachim]

I found two issues with this patch regarding the default clause:

• The spec does not require a default clause. I get error: expected expression if I omit a default clause. The error is gone if I add default().
• The spec does not allow an empty default() clause, but rather expects default(nothing)or omission of the default clause. This patch accepts the empty default clause.

[jdoerfert]

In D91944#2504924, @protze.joachim wrote:

I found two issues with this patch regarding the default clause:

• The spec does not require a default clause. I get error: expected expression if I omit a default clause. The error is gone if I add default().
• The spec does not allow an empty default() clause, but rather expects default(nothing)or omission of the default clause. This patch accepts the empty default clause.

There are various issues with this patch. We will fix them afterwards. The patch provides a lot of infrastructure we need. Please provide the problematic cases in a bug.

[jdoerfert]

In D91944#2485167, @jdoerfert wrote:

Are the test all passing?

Can we fix the failing tests so we can merge this?

[alokmishra.besu]

In D91944#2505159, @jdoerfert wrote:

In D91944#2485167, @jdoerfert wrote:

Are the test all passing?

Can we fix the failing tests so we can merge this?

I will work on it this week. Will update soon.

[alokmishra.besu]

I've rebase this patch with the latest git code.
All test cases pass.
I've also applied this patch to a new git clone. It applies and builds successfully.

[jdoerfert]

There are no test cases anymore, as far as I can tell.

[alokmishra.besu]

In D91944#2540178, @jdoerfert wrote:

There are no test cases anymore, as far as I can tell.

Updated patch.

[jdoerfert]

Asssuming this passes the tests I'll merge it as is. There are unresolved review requests, e.g., custom implementation instead of balanced parenthesis trackers, the template support, etc.
but we decided we can make faster progress in tree once this is merged.