[PATCH] D109635: [WIP][OpenMP] Support construct trait set for Clang

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Cool!

We certainly need more tests but the design you came up with is certainly nicer than what I had in mind.
One thing I wanted to do though is keep track of the constructs in the OpenMPIRBuilder instead. So move the

  /// Vector of declare variant construct traits.
  SmallVector<llvm::omp::TraitProperty, 8> ConstructTraits;

into OpenMPIRBuilder and add/delete the things there. It's not a conceptual difference but makes it easier
to opt-in for Flang later.

I also left some comments below. I don't think there is anything major though.



================
Comment at: clang/lib/AST/OpenMPClause.cpp:2346
 
-      VMI.ConstructTraits.push_back(Selector.Properties.front().Kind);
+      // VMI.ConstructTraits.push_back(Selector.Properties.front().Kind);
     }
----------------
cchen wrote:
> I'm now removing this line since the properties are already added at line 2334 and not removing this line would lead to an extra iteration in `OMPContext.cpp` (check out the second diff), and the code will always return false even if the construct trait properties are added.
> 
> Diff for OpenMPClause.cpp
> ```
> 2333       for (const OMPTraitProperty &Property : Selector.Properties)
> 2334         VMI.addTrait(Set.Kind, Property.Kind, Property.RawString, ScorePtr);
> 2335
> 2336       if (Set.Kind != TraitSet::construct)
> 2337         continue;
> 2338
> 2339       // TODO: This might not hold once we implement SIMD properly.
> 2340       assert(Selector.Properties.size() == 1 &&
> 2341              Selector.Properties.front().Kind ==
> 2342                  getOpenMPContextTraitPropertyForSelector(
> 2343                      Selector.Kind) &&
> 2344              "Ill-formed construct selector!");
> 2345
> 2346       // VMI.ConstructTraits.push_back(Selector.Properties.front().Kind);
> ```
> 
> Diff for `lib/Frontend/OpenMP/OMPContext.cpp`:
> ```
> 224     unsigned ConstructIdx = 0, NoConstructTraits = Ctx.ConstructTraits.size();
> 225     for (TraitProperty Property : VMI.ConstructTraits) {
> 226       assert(getOpenMPContextTraitSetForProperty(Property) ==
> 227                  TraitSet::construct &&
> 228              "Variant context is ill-formed!");
> 229
> 230       // Verify the nesting.
> 231       bool FoundInOrder = false;
> 232       while (!FoundInOrder && ConstructIdx != NoConstructTraits)
> 233         FoundInOrder = (Ctx.ConstructTraits[ConstructIdx++] == Property);
> 234       if (ConstructMatches)
> 235         ConstructMatches->push_back(ConstructIdx - 1);
> 236
> 237       Optional<bool> Result = HandleTrait(Property, FoundInOrder);
> 238       if (Result.hasValue())
> 239         return Result.getValue();
> 240
> 241       if (!FoundInOrder) {
> 242         LLVM_DEBUG(dbgs() << "[" << DEBUG_TYPE << "] Construct property "
> 243                           << getOpenMPContextTraitPropertyName(Property, "")
> 244                           << " was not nested properly.\n");
> 245         return false;
> 246       }
> 247
> 248       // TODO: Verify SIMD
> 249     }
> ```
I'm not surprised some things need adjustment. Never tested construct trait matching much as we did not expose it via clang. Just delete the line if you think it's wrong. Also consider deleting the code before if it's not needed.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:737
+  }
+  void eraseConstructTrait() { ConstructTraits.clear(); }
+
----------------
Modification was made to make it clear what happens but now I think this is not needed.

See below first. Maybe the interface should be "handleConstructTrait(Property, bool Insert)" and if Insert is false you verify it's the last one and delete it. You'd need to traverse them in opposite order though to verify, unsure if verification is therefore strictly necessary.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3894-3896
+  if (isOpenMPWorksharingDirective(DKind)) {
+    Stack->addConstructTrait(llvm::omp::TraitProperty::construct_for_for);
+  }
----------------



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4491
                                       ArrayRef<OMPClause *> Clauses) {
+  DSAStack->eraseConstructTrait();
   if (DSAStack->getCurrentDirective() == OMPD_atomic ||
----------------
I know think you need to call something like `addDeclareVariantConstructTrait` again and do pop_back() instead of push_back() for each trait matching the DKind. Clearing, or erasing a fixed amount of traits, does not work. 


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6838
+  for (auto Property : DSAStack->getConstructTraits())
+    OMPCtx.addTrait(Property);
 
----------------
Maybe we should pass the traits to the constructor, makes it clearer they have to be passed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109635