[PATCH] D77812: [flang] Semantic checks for OpenMP combined constructs.

[David Truby via Phabricator via llvm-commits]

DavidTruby marked 4 inline comments as done.
DavidTruby added inline comments.


================
Comment at: flang/include/flang/Common/enum-set.h:115
+  constexpr EnumSet operator+(enumerationType v) const {
+    EnumSet result = *this | EnumSet{v};
+    return result;
----------------
klausler wrote:
> Please be consistent with the braced initialization used throughout f18.  It's safer for many types than old-style initialization.
Looking at it again I don't think the intermediate result is necessary anyway. I can just return the expression.


================
Comment at: flang/include/flang/Common/enum-set.h:119
+  constexpr EnumSet operator-(enumerationType v) const {
+    EnumSet result = *this ^ EnumSet { v };
+    return result;
----------------
klausler wrote:
> That's going to add `v` to the set if it's not already present, which seems unintended.
Good spot. I believe it should be `*this & ~EnumSet{v};`?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:16
 
+static constexpr inline OmpClauseSet doAllowedClauses{OmpClause::PRIVATE,
+    OmpClause::FIRSTPRIVATE, OmpClause::LASTPRIVATE, OmpClause::LINEAR,
----------------
klausler wrote:
> What do you think `inline` is going to do for you when `static` also appears?
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.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:30
+    OmpClause::PRIVATE, OmpClause::FIRSTPRIVATE, OmpClause::SHARED,
+    OmpClause::COPYIN, OmpClause::REDUCTION};
+static constexpr inline OmpClauseSet parallelAllowedOnceClauses{
----------------
jdoerfert wrote:
> 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); 
> ```
> 
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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77812/new/

https://reviews.llvm.org/D77812