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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added a comment.

@jdoerfert To answer your question as to why it is being codegened as shared, codegen doesn't handle `default` clause- at least I didn't come across such a thing- and if you think about it, it shouldn't need to. You either have `none` or `shared`:

- `none` is largly to throw diag errors - so clang either never leaves sema because it threw an error, or there are no errors, which suggests that the programmer has already listed all captured variables as other clauses. - either way, codegen doesn't need to handle it.
- `shared` is the default anyway, and codegen already makes any captured variableby the outlined function not "claimed" by any other privatization clause, shared - it just passes it as an arg. to the outlined function.

and Since @atmnpatel didn't add any new codegen for the `default{firstprivate}` case, codegen defaulted to shared - at least that's what I think happened.

@atmnpatel  The way privatization works currently in codegen, you go over every clause, find the list of variables for that clause, find some relevant info about the variable along with the proper way to initialize the variable (i.e. what constructor to use), and add it to list of all variables to be privatized, once done with all privatization clauses, you do codegen for privatization for all of them in one go. The exception to this is `copyin`. 
So what you may want to do is to handle the `default` clause in codegen. if it's `none` or `shared`, you ignore it. if it is `firstprivate` or `private`, find the captured variables of the soon to be outlined function that have not been captured by any other privatization clause, then figure a way to pass it to `codegenfunction::emitOMPFirstprivateClause` or  `codegenfunction::emitPrivateClause`. Alternatively, find this list while building the AST, and add them as default captured variables, which will make codegen much easier. You are already checking for them to throw the `none` diag error anyway.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7087
 
+/// Matches if the OpenMP ``default`` clause has ``shared`` kind specified.
+///
----------------
fix comment: ... has ``firstprivate`` kind ...


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4993
 
   // Check variables in the clauses if default(none) was specified.
+  if (DSAStack->getDefaultDSA() == DSA_none ||
----------------
fix the comments to say firstprivate as well (here and elsewhere)


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5102
       Diag(P.second->getExprLoc(), diag::err_omp_no_dsa_for_variable)
           << P.first << P.second->getSourceRange();
       Diag(DSAStack->getDefaultDSALocation(), diag::note_omp_default_dsa_none);
----------------
Why is `firstprivate` throwing this error? isn't the purpose of specifying it as `default` is if a variable is not specified as anything, then it is automatically handled as `firstprivate`? or am I misunderstanding something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591