[flang] Semantic checks for OpenMP combined constructs.

[DavidTruby]

Summary

This includes a refactor of the existing combined construct checks
that were present, as well as adding the remaining combined constructs
that had not been implemented yet.

[DavidTruby]

flang/include/flang/Common/enum-set.h

1

I have no idea what's going on with these lints. git-clang-format doesn't suggest any changes, and this is suggesting an empty change?

[DavidTruby]

This review is continued from https://github.com/flang-compiler/f18/pull/987

[jdoerfert]

flang/lib/Semantics/check-omp-structure.cpp

30

This is the kind of information I would like to share with OMPKinds.def. While we need to add the "once" flag there the interface in OMPConstants.h is already there to be reused:

/// Return true if \p C is a valid clause for \p D in version \p Version.                                             
bool isAllowedClauseForDirective(Directive D, Clause C, unsigned Version);

[jdoerfert]

My other comment was of general nature not to block this review.

flang/include/flang/Common/enum-set.h

1

Maybe your local clang-format returns an error, e.g., it is not found? I'm guessing here.

[klausler]

flang/include/flang/Common/enum-set.h

115

Please be consistent with the braced initialization used throughout f18. It's safer for many types than old-style initialization.

119

That's going to add v to the set if it's not already present, which seems unintended.

flang/lib/Semantics/check-omp-structure.cpp

16

What do you think inline is going to do for you when static also appears?

[DavidTruby]

flang/include/flang/Common/enum-set.h

115

Looking at it again I don't think the intermediate result is necessary anyway. I can just return the expression.

119

Good spot. I believe it should be *this & ~EnumSet{v};?

flang/lib/Semantics/check-omp-structure.cpp

16

I'm really using inline here as a hint to the reader/future contributors of the intent of these variables. It's certainly not necessary, as it is implied by constexpr and the semantics it provides on top of static aren't necessary here.
I'm happy to remove it but I do think it signals the intent of what I'm doing.

30

I agree that it would be ideal to share this information between different parts of the LLVM project. However as alluded to in your email to flang-dev we would need to reconcile the difference between Flang's OmpDirective and OmpClause and the Directive and Clause classes in this function. I think that's outside the scope of this patch but certianly something we should consider in future!

[klausler]

flang/lib/Semantics/check-omp-structure.cpp

16

What is the hint to the reader?

[DavidTruby]

flang/lib/Semantics/check-omp-structure.cpp

16

That the variables shouldn't be used in any context that would cause them to have to exist in the binary

[klausler]

Is it possible to see an updated diff with fixes?

[DavidTruby]

In D77812#1978420, @klausler wrote:

Is it possible to see an updated diff with fixes?

Sure, I'll update it tomorrow; Friday and Today are non-working days here in the UK.

[DavidTruby]

Fix EnumSet bit handling.

[DavidTruby]

@klausler do you want me to make any more changes to this?