Page MenuHomePhabricator

[flang] Semantic checks for OpenMP combined constructs.
ClosedPublic

Authored by DavidTruby on Apr 9 2020, 10:55 AM.

Details

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.

Diff Detail

Event Timeline

DavidTruby created this revision.Apr 9 2020, 10:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
DavidTruby added inline comments.
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?

jdoerfert added inline comments.Apr 9 2020, 1:13 PM
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 resigned from this revision.Apr 9 2020, 8:09 PM

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 requested changes to this revision.Apr 10 2020, 2:19 PM
klausler added a subscriber: klausler.
klausler added inline comments.
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?

This revision now requires changes to proceed.Apr 10 2020, 2:19 PM
DavidTruby marked 4 inline comments as done.Apr 10 2020, 4:56 PM
DavidTruby added inline comments.
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 added inline comments.Apr 13 2020, 9:19 AM
flang/lib/Semantics/check-omp-structure.cpp
16

What is the hint to the reader?

DavidTruby marked an inline comment as done.Apr 13 2020, 11:09 AM
DavidTruby added inline comments.
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

Is it possible to see an updated diff with fixes?

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.

Fix EnumSet bit handling.

ChinouneMehdi added a project: Restricted Project.Apr 15 2020, 5:45 AM
DavidTruby marked 2 inline comments as done.Apr 15 2020, 6:22 AM

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

klausler accepted this revision.Apr 24 2020, 8:45 AM
This revision is now accepted and ready to land.Apr 24 2020, 8:45 AM
This revision was automatically updated to reflect the committed changes.