[PATCH] D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang

[Atmn Patel via Phabricator via cfe-commits]

atmnpatel added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3434-3435
+      if (Stack->getDefaultDSA() == DSA_firstprivate &&
+          VD->getStorageDuration() == SD_Static &&
+          CanonicalVD->getDeclContext()->isFileContext() &&
           !Stack->isLoopControlVariable(VD).first) {
----------------
ABataev wrote:
> Hmm, maybe move this check to `getDSA()`? If you do it, you can just modify the check on line 3322 
> ```
>       if (DVar.CKind == OMPC_unknown && (Stack->getDefaultDSA() == DSA_none || (Stack->getDefaultDSA() == DSA_firstprivate && !Stack->isLoopControlVariable(VD).first)) &&
>           isImplicitOrExplicitTaskingRegion(DKind) &&
>           VarsWithInheritedDSA.count(VD) == 0) {
>         VarsWithInheritedDSA[VD] = E;
>         return;
>       }
> ```
Didn't we move it this far down to avoid processing it before all of the target-directive relevant checks?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3441-3446
+      // Create implicit firstprivate variables as necessary under
+      // default(firstprivate).
+      if (Stack->getDefaultDSA() == DSA_firstprivate) {
         ImplicitFirstprivate.push_back(E);
         return;
       }
----------------
ABataev wrote:
> Hmm, not sure that this check is needed here, the next if statement should handle it already, no? `DVar.CKind` is set to `OMPC_firstprivate` and the next check must work.
It doesn't work directly but I can simplify it further and merge them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591