[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Basic/CMakeLists.txt:4
   TargetParser
+  FrontendOpenMP
   )

What requires this new dependency?


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2516
 
+extern bool checkFailParameter(OpenMPClauseKind FailParameter);
+/// This represents 'fail' clause in the '#pragma omp atomic'

No way for extern functions. Declare in include/clang/Basic/OpenMPKinds.h



Comment at: clang/include/clang/AST/OpenMPClause.h:2544-2545
+
+if (!checkFailParameter(FailParameter))
+  llvm_unreachable("Invalid fail clause parameter");
+  }





Comment at: clang/lib/Sema/SemaOpenMP.cpp:17707-17714
+bool clang::checkFailParameter(OpenMPClauseKind FailParameter) {
+  if (FailParameter == llvm::omp::OMPC_acquire ||
+  FailParameter == llvm::omp::OMPC_relaxed ||
+  FailParameter == llvm::omp::OMPC_seq_cst)
+return true;
+
+  return false;

Move it to lib/Basic/OpenMPKinds.cpp



Comment at: clang/lib/Sema/SemaOpenMP.cpp:17721
+
+  if (!clang::checkFailParameter(Parameter)) {
+Diag(KindLoc, diag::err_omp_atomic_fail_wrong_or_no_clauses);




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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2554
+  break;
+case llvm::omp::OMPC_adjust_args:
+case llvm::omp::OMPC_affinity:

I think all these cases are unexpected and must be terminated with 
llvm_unreachable


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D123235#4656464 , @koops wrote:

> In clang/lib/AST/OpenMPClause.cpp,
>
>   OMPClauseWithPreInit::get(const OMPClause *C) {
>   switch(C->getClauseKind()) {
>   case OMPC_schedule:
>   
>   default:
>   break;
>   }
>
> It is not possible to list down all possible ``OpenMPClauseKind``` types in 
> the switch. Hence they have used a default even in other methods. I am 
> following the same logic.

Why not possible?


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2555
+  break;
+default:
+  this->FailParameter = llvm::omp::OMPC_unknown;

Same here, remove default: and explicitly list all possible values


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D123235#4656459 , @koops wrote:

> After the reverting of changes, putting the default in the switch case as 
> discussed in the comments earlier.

No, do not put the default: switch, instead you need explicitly list all other 
enumeric values, like

  case 1:
  case 2:
break;


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: clang/lib/Sema/SemaOpenMP.cpp:12693
+  OpenMPClauseKind FailParameter = FC->getFailParameter();
+  SourceLocation DisplayLocation = (FailParameter == OMPC_unknown)
+   ? FC->getBeginLoc()

Same, remove parens


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12694-12695
+  SourceLocation DisplayLocation =
+  ((FailParameter == OMPC_unknown) ? FC->getBeginLoc()
+   : FC->getFailParameterLoc());
+  if ((FailParameter != OMPC_acq_rel) && (FailParameter != OMPC_acquire) &&

Remove extra parens



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12696-12698
+  if ((FailParameter != OMPC_acq_rel) && (FailParameter != OMPC_acquire) &&
+  (FailParameter != OMPC_relaxed) && (FailParameter != OMPC_release) &&
+  (FailParameter != OMPC_seq_cst))

Remove extra parens, LLVM does not use this style


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3837-3838
 return nullptr;
-  return Actions.ActOnOpenMPClause(Kind, Loc, Tok.getLocation());
+  OMPClause *Clause = Actions.ActOnOpenMPClause(Kind, Loc, Tok.getLocation());
+  return Clause;
 }





Comment at: clang/lib/Sema/SemaOpenMP.cpp:12684
+  }
+  const auto *FC = dyn_cast(C);
+  OpenMPClauseKind FailParameter = FC->getFailParameter();





Comment at: clang/lib/Sema/SemaOpenMP.cpp:12689-12693
+  if (!((FailParameter == OMPC_acq_rel) ||
+(FailParameter == OMPC_acquire) ||
+(FailParameter == OMPC_relaxed) ||
+(FailParameter == OMPC_release) ||
+(FailParameter == OMPC_seq_cst))) {





Comment at: clang/lib/Sema/SemaOpenMP.cpp:12693-12695
+(FailParameter == OMPC_seq_cst))) {
+Diag(DisplayLocation, diag::err_omp_atomic_fail_wrong_or_no_clauses);
+  }

No need for braces here


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2561
+
+  void initFailClause(SourceLocation LParenLoc, OpenMPClauseKind FailParameter,
+  SourceLocation FailParameterLoc) {

Unused?



Comment at: clang/include/clang/AST/OpenMPClause.h:2590-2597
+  static OMPFailClause *CreateEmpty(const ASTContext &C);
+  static OMPFailClause *Create(const ASTContext &C, SourceLocation StartLoc,
+   SourceLocation EndLoc);
+  static OMPFailClause *Create(const ASTContext &C,
+   OpenMPClauseKind FailParameter,
+   SourceLocation FailParameterLoc,
+   SourceLocation StartLoc,

You don't need all these function, this clause is a simple one, it does not 
require special create function, just new (...) OMPFailClause works in all cases



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12632
 
+void Sema::checkFailClauseParameters(ArrayRef Clauses) {
+  for (const OMPClause *C : Clauses) {

You already has all the info for the fail clause, why do you need to scan 
through the list of all available clauses to find this fail clause?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12637
+  continue;
+/* Clauses contains OMPC_fail and the subclause */
+OpenMPClauseKind FailParameter = FC->getFailParameter();

Use С++ style comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12708
+  SubClauseLoc = C->getBeginLoc();
+  break;
+}

You can perform all required checks for the fail clause here



Comment at: clang/lib/Sema/SemaOpenMP.cpp:17723
+   SourceLocation EndLoc) {
+  return OMPFailClause::Create(Context, StartLoc, EndLoc);
+}

Just new (Context) OMPFailClause(StartLoc, EndLoc);



Comment at: clang/lib/Serialization/ASTReader.cpp:10278
+  case llvm::omp::OMPC_fail:
+C = OMPFailClause::CreateEmpty(Context);
+break;

Just С = new (Context) OMPFailClause();


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3328-3332
 if (!FirstClause) {
   Diag(Tok, diag::err_omp_more_one_clause)
   << getOpenMPDirectiveName(DKind) << getOpenMPClauseName(CKind) << 0;
   ErrorFound = true;
 }

If there is a second instance of this clause, it should be reported here



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3830
+  // Store fail parameter for Sema.
+  FailClause->initFailClause(LParenLoc, CKind, MemOrderLoc);
+

That's a bad place, it should be done in Sema.


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2543
+  void setFailParameter(OpenMPClauseKind FailParameter) {
+
+switch (FailParameter) {

Remove this empty line



Comment at: clang/include/clang/AST/OpenMPClause.h:2611-2617
+  void initFailClause(SourceLocation LParenLoc, OpenMPClauseKind FailParameter,
+  SourceLocation FailParameterLoc) {
+
+setLParenLoc(LParenLoc);
+setFailParameterLoc(FailParameterLoc);
+setFailParameter(FailParameter);
+  }

make this function private



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10964
+def err_omp_atomic_fail_wrong_or_no_clauses : Error<"expected a memory order 
clause">;
+def err_omp_atomic_fail_extra_mem_order_clauses : Error<"directive '#pragma 
omp atomic compare fail' cannot contain more than one memory order clause">;
+def err_omp_atomic_fail_extra_clauses : Error<"directive '#pragma omp atomic 
compare' cannot contain more than one fail clause">;

Unused error message?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10965
+def err_omp_atomic_fail_extra_mem_order_clauses : Error<"directive '#pragma 
omp atomic compare fail' cannot contain more than one memory order clause">;
+def err_omp_atomic_fail_extra_clauses : Error<"directive '#pragma omp atomic 
compare' cannot contain more than one fail clause">;
+def err_omp_atomic_fail_no_compare : Error<"expected 'compare' clause with the 
'fail' modifier">;

I thin k this must be handled by parse..clause function already


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2525
+
+  OMPClause *FailMemoryOrderClause = nullptr;
+  SourceLocation ArgumentLoc;

I don't like the idea of a reference to another clause here, it may lead to 
many issues with serialization/deserialization, use-after-free etc. Better to 
keep a flag (kind of memory order clause?) that there is associated clause here 
and then just find it in the list of clauses, where required.


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: [OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/ASTNodeTraverser.h:217-220
+if (const auto *OMPC = dyn_cast(C)) {
+  Visit(OMPC);
+  return;
+}

Why do you need special logic here?



Comment at: clang/include/clang/AST/OpenMPClause.h:2320
+
+  OMPClause *MemoryOrderClause = nullptr;
+  OpenMPClauseKind FailParameter = llvm::omp::OMPC_unknown;

That's not the best decision, better to make this kind of clause a base class 
for these new class.



Comment at: clang/include/clang/AST/OpenMPClause.h:2373
+SourceLocation StartLoc, SourceLocation LParenLoc,
+   SourceLocation EndLoc)
+  : OMPClause(llvm::omp::OMPC_fail, StartLoc, EndLoc), 
ArgumentLoc(ArgumentLoc), LParenLoc(LParenLoc)  {

Formatting



Comment at: clang/include/clang/AST/OpenMPClause.h:2429
+
+  const OMPClause *const_getMemoryOrderClause() const {
+return static_cast(MemoryOrderClause);

No need for const_ prefix in the name


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: [OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6181-6190
+  checkNestingOfRegions(*this, DSAStack, Kind, DirName, CancelRegion,
+BindKind, StartLoc);
   Kind = OMPD_for;
   DSAStack->setCurrentDirective(OMPD_for);
   DSAStack->setMappedDirective(OMPD_loop);
   PrevMappedDirective = OMPD_loop;
   break;

koops wrote:
> ABataev wrote:
> > Need to drop these extra calls of checkNestingOfRegions(), all check must 
> > be handled in the single call. Can we do it?
> 1) Default binding in "#pragma omp loop" is handled in the beginning of 
> mapLoopConstruct.
> #pragma omp parallel
> 
> #pragma omp loop
> ...
> indicates "omp loop" will be "omp loop bind(parallel)". So, I cannot call 
> checkNestingOfRegions() before call to mapLoopConstruct()
> 2) In mapLoopConstruct the directive/construct of "omp loop" is changed to 
> other directives like "omp for". So, it is not possible to call 
> checkNestingOfRegions() after the call to mapLoopConstruct.
> Because of the above 2 reasons the current code seems correct. Please let me 
> know if you have any alternative in mind.
Can you perform mapLoopConstruct earlier, before  the very first 
checkNestingOfRegions()? Current code structure ovrcomplicates the code, makes 
it much harder to understand and to maintain


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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: [OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6181-6190
+  checkNestingOfRegions(*this, DSAStack, Kind, DirName, CancelRegion,
+BindKind, StartLoc);
   Kind = OMPD_for;
   DSAStack->setCurrentDirective(OMPD_for);
   DSAStack->setMappedDirective(OMPD_loop);
   PrevMappedDirective = OMPD_loop;
   break;

Need to drop these extra calls of checkNestingOfRegions(), all check must be 
handled in the single call. Can we do it?


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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: [OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6230
 BindKind = BC->getBindKind();
+
   // First check CancelRegion which is then used in checkNestingOfRegions.

Remove this new line



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6245-6248
+  UseClausesWithoutBind = mapLoopConstruct(
+  ClausesWithoutBind, Clauses, BindKind, Kind, PrevMappedDirective,
+  StartLoc, EndLoc, DirName, CancelRegion);
 

Can we do this before checks in lines 6232-6235 so we can avoid double call of 
checkNestingOfRegions for bind?


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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: [OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6167-6183
+  if (isOpenMPWorksharingDirective(ParentDirective) ||
+  ParentDirective == OMPD_loop) {
+Diag(StartLoc, diag::err_omp_prohibited_region)
+<< true << getOpenMPDirectiveName(ParentDirective) << 1
+<< getOpenMPDirectiveName(Kind);
+  }
   Kind = OMPD_for;

Can you move these checks to checkNestingOfRegions() function?


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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: [OpenMP] Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D158266#4640416 , @koops wrote:

> Can someone please review the changes that I uploaded last week?

Still need to add extra tests to the nesting_of_regions.cpp test


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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158778: [CUDA] Propagate __float128 support from the host.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D158778#4622901 , @tra wrote:

> @ABataev
>
> This patch breaks breaks two tests:
>
> - 
> github.com/llvm/llvm-project/blob/main/clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp
> - 
> github.com/llvm/llvm-project/blob/main/clang/test/OpenMP/nvptx_unsupported_type_messages.cpp
>
> It's not clear what exactly these tests are testing for and I can't tell 
> whether I should just remove the checks related to `__float128`, or if 
> there's something else that would need to be done on the OpenMP side.
>
> AFAICT, OpenMP will pick up `double` format for `__float128` after my patch. 
> This suggests that we would only have `long double` left as an unsupported 
> type on GPU-supporting targets, which suggests that I should just remove the 
> checks related to `__float128` from those tests.
>
> Am I missing something? Is there anything else that may need to be done on 
> the OpenMP side?

Just checks removal should be fine


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158778

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_simd_tl_codegen.cpp:30
+// OMP51-NEXT:  entry:
+// OMP51-NEXT:[[DOTGLOBAL_TID__ADDR_I:%.*]] = alloca i32, align 4
+// OMP51-NEXT:[[DOTPART_ID__ADDR_I:%.*]] = alloca ptr, align 8

sandeepkosuri wrote:
> ABataev wrote:
> > sandeepkosuri wrote:
> > > ABataev wrote:
> > > > Why removed these checks?
> > > I did not remove any check lines in this function.
> > > But I removed checks in `omp_task_entry` function that were not related 
> > > to my changes, to avoid failures. I only wanted to check whether 
> > > `__kmpc_set_thread_limit()` is called.
> > > 
> > > Same for all the other test cases.
> > Better to restore it to be able to use the script in future without many 
> > changes
> But a few check lines are failing on windows, while passing on debian.
It must be investigated, it is bad idea just to remove these checks. Most 
probably related to the order of the expressions emission.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_simd_tl_codegen.cpp:30
+// OMP51-NEXT:  entry:
+// OMP51-NEXT:[[DOTGLOBAL_TID__ADDR_I:%.*]] = alloca i32, align 4
+// OMP51-NEXT:[[DOTPART_ID__ADDR_I:%.*]] = alloca ptr, align 8

sandeepkosuri wrote:
> ABataev wrote:
> > Why removed these checks?
> I did not remove any check lines in this function.
> But I removed checks in `omp_task_entry` function that were not related to my 
> changes, to avoid failures. I only wanted to check whether 
> `__kmpc_set_thread_limit()` is called.
> 
> Same for all the other test cases.
Better to restore it to be able to use the script in future without many changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_simd_tl_codegen.cpp:30
+// OMP51-NEXT:  entry:
+// OMP51-NEXT:[[DOTGLOBAL_TID__ADDR_I:%.*]] = alloca i32, align 4
+// OMP51-NEXT:[[DOTPART_ID__ADDR_I:%.*]] = alloca ptr, align 8

Why removed these checks?



Comment at: clang/test/OpenMP/target_parallel_for_tl_codegen.cpp:30
+// OMP51-NEXT:  entry:
+// OMP51-NEXT:[[DOTGLOBAL_TID__ADDR_I:%.*]] = alloca i32, align 4
+// OMP51-NEXT:[[DOTPART_ID__ADDR_I:%.*]] = alloca ptr, align 8

Same, why it was removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D158559#4608410 , 
@ivanrodriguez3753 wrote:

> Also, should the underlying issue and test case be filed as an issue on 
> Github? I wasn't sure since this revision includes the bug and a description

If you're going to fix this yourself - probably it is not necessary to create a 
bug report. Otherwise, please go ahead and create a bug report.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D158559#4608397 , 
@ivanrodriguez3753 wrote:

> In D158559#4608388 , @ABataev wrote:
>
>> 1. Always provide full context in the patch.
>
> Sure, would you mind mentioning what's missing? Or do you mean the stuff from 
> debug output being shortened, as well as LLVM IR being only snippets?

I mean, provide full diff context, read the docs how to upload patches for the 
review. 
https://www.llvm.org/docs/Phabricator.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158559: [OpenMP] WIP: Attempt to fix clang frontend codegen issue

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

1. Always provide full context in the patch.
2. It looks like we're counting the pointer size (or the size of just the first 
element of the array), because  we do not account array section here, either 
just the pointer or the first element only (most probably second option). Need 
to teach the compiler about array section here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158559

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157933: [OpenMP 5.1] Parsing and Sema support for `scope` construct

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG. Please, update docs/OpenMPSupport.rst


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

https://reviews.llvm.org/D157933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:18098-18101
   case OMPC_allocate:
 Res = ActOnOpenMPAllocateClause(Data.DepModOrTailExpr, VarList, StartLoc,
-LParenLoc, ColonLoc, EndLoc);
+ColonLoc, LParenLoc, EndLoc);
 break;

steakhal wrote:
> Looks like another TP.
> WDYT @ABataev
Yes, this is the correct fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Add tests for the nesting of regions to check all possible combinations 
correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158266: Patch for Support to loop bind clause : Checking Parent Region

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.h:251
+/// or 'omp for' directive, otherwise - false.
+bool isOpenMPForDirective(OpenMPDirectiveKind DKind);
+

What if the outer regioun is sections?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157933: [OpenMP 5.1] Parsing and Sema support for `scope` construct

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Could you also add the nesting tests for outer scope directive? Currently it 
tests only for inner


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

https://reviews.llvm.org/D157933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157933: [OpenMP 5.1] Parsing and Sema support for `scope` construct

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D157933#4587164 , @mdfazlay wrote:

> In D157933#4586816 , @ABataev wrote:
>
>> Need to add the tests (and the checks, if required) for the nesting of the 
>> regions
>
> I think I have the nesting of regions checks in //scope_messages.cpp//. Do 
> you prefer to have those checks in //nesting_of_regions.cpp// file?

Yes, extend this one


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157933: [OpenMP 5.1] Parsing and Sema support for `scope` construct

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Need to add the tests (and the checks, if required) for the nesting of the 
regions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157933

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1570-1575
+  // If we are here with a 'target teams loop' then we are emitting the
+  // 'parallel' region of the 'target teams distribute parallel for'
+  // emitted in place of the 'target teams loop'. Based on the specification
+  // noted above, an if-clause associated with a 'target teams loop', be it
+  // 'if(val)' or an 'if(target:val)', will apply only to 'target' and not
+  // the 'parallel' of the 'target teams distribute parallel for'.

ddpagan wrote:
> ABataev wrote:
> > ddpagan wrote:
> > > ABataev wrote:
> > > > ddpagan wrote:
> > > > > ABataev wrote:
> > > > > > It does not match the spec. 
> > > > > > ```
> > > > > > For a combined or composite construct, if no 
> > > > > > directive-name-modifier is specified then the if clause applies to 
> > > > > > all constituent constructs to which an if clause can apply.
> > > > > > ```
> > > > > > So, if(val) should be applied to both target and parallel regions, 
> > > > > > no?
> > > > > > It does not match the spec. 
> > > > > > ```
> > > > > > For a combined or composite construct, if no 
> > > > > > directive-name-modifier is specified then the if clause applies to 
> > > > > > all constituent constructs to which an if clause can apply.
> > > > > > ```
> > > > > > So, if(val) should be applied to both target and parallel regions, 
> > > > > > no?
> > > > > 
> > > > > Hi Alexey - Question for you: does revising the comment above at 
> > > > > lines 1570-1575 to the following text help explain in a better way 
> > > > > what's being done, and why?
> > > > > 
> > > > >   If we are handling a 'target teams distribute parallel for' 
> > > > > explicitly written
> > > > >   in the source, and it has an 'if(val)' clause, the if condition is 
> > > > > applied to
> > > > >   both 'target' and 'parallel' regions according to
> > > > >   OpenMP 5.2 [3.4, if Clause, Semantics, 15-18].
> > > > > 
> > > > >   However, if we are mapping an explicit 'target teams loop if(val)' 
> > > > > onto a
> > > > >   'target teams distribute parallel for if(val)', to preserve the 
> > > > > 'if' semantics
> > > > >   as specified by the user with the 'target teams loop', we apply it 
> > > > > just to
> > > > >   the 'target' region.
> > > > It does not match the spec. Why we shall handle it this way?
> > > You're right, Alexey ... it doesn't match the spec, but here's why we 
> > > thought this would be an appropriate way to implement 'target teams loop 
> > > if(val)'. When a user specifies 'if(val)' with a 'target teams loop', 
> > > their expectation is that its effect will only apply to the 'target' 
> > > region. Since a 'loop' construct can be implemented in a number of 
> > > different ways with the freedom granted by the specs description of 
> > > 'loop' (part of which describes it as being able to be run concurrently), 
> > > using a 'target teams distribute parallel for' construct is a simple and 
> > > effective default choice, which is what happens today. 
> > > target_teams_loop => target_teams_distribute_parallel_for
> > > Applying the if clause to the parallel region for this case can 
> > > potentially limit it to one thread, which would hinder performance gains 
> > > otherwise possible, and presumably wouldn't be what the user 
> > > wanted/expected.
> > > 
> > > The semantics of the spec (OpenMP 5.2 [3.4, if Clause, Semantics, 15-18]) 
> > > is definitely what should be applied to an explicit instance of 
> > > target_teams_distribute_parallel_for, but in this case (when mapping 
> > > target_teams_loop => target_teams_distribute_parallel_for) it seems 
> > > reasonable to make the choice described above.
> > I'm not sure this is true users  expectations.
> You're right in the sense that there is an assumption being made there. So 
> would you recommend just staying with the semantics as defined in the spec 
> regardless?
Yew, better to follow spec defaults.


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

https://reviews.llvm.org/D157197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1570-1575
+  // If we are here with a 'target teams loop' then we are emitting the
+  // 'parallel' region of the 'target teams distribute parallel for'
+  // emitted in place of the 'target teams loop'. Based on the specification
+  // noted above, an if-clause associated with a 'target teams loop', be it
+  // 'if(val)' or an 'if(target:val)', will apply only to 'target' and not
+  // the 'parallel' of the 'target teams distribute parallel for'.

ddpagan wrote:
> ABataev wrote:
> > ddpagan wrote:
> > > ABataev wrote:
> > > > It does not match the spec. 
> > > > ```
> > > > For a combined or composite construct, if no directive-name-modifier is 
> > > > specified then the if clause applies to all constituent constructs to 
> > > > which an if clause can apply.
> > > > ```
> > > > So, if(val) should be applied to both target and parallel regions, no?
> > > > It does not match the spec. 
> > > > ```
> > > > For a combined or composite construct, if no directive-name-modifier is 
> > > > specified then the if clause applies to all constituent constructs to 
> > > > which an if clause can apply.
> > > > ```
> > > > So, if(val) should be applied to both target and parallel regions, no?
> > > 
> > > Hi Alexey - Question for you: does revising the comment above at lines 
> > > 1570-1575 to the following text help explain in a better way what's being 
> > > done, and why?
> > > 
> > >   If we are handling a 'target teams distribute parallel for' explicitly 
> > > written
> > >   in the source, and it has an 'if(val)' clause, the if condition is 
> > > applied to
> > >   both 'target' and 'parallel' regions according to
> > >   OpenMP 5.2 [3.4, if Clause, Semantics, 15-18].
> > > 
> > >   However, if we are mapping an explicit 'target teams loop if(val)' onto 
> > > a
> > >   'target teams distribute parallel for if(val)', to preserve the 'if' 
> > > semantics
> > >   as specified by the user with the 'target teams loop', we apply it just 
> > > to
> > >   the 'target' region.
> > It does not match the spec. Why we shall handle it this way?
> You're right, Alexey ... it doesn't match the spec, but here's why we thought 
> this would be an appropriate way to implement 'target teams loop if(val)'. 
> When a user specifies 'if(val)' with a 'target teams loop', their expectation 
> is that its effect will only apply to the 'target' region. Since a 'loop' 
> construct can be implemented in a number of different ways with the freedom 
> granted by the specs description of 'loop' (part of which describes it as 
> being able to be run concurrently), using a 'target teams distribute parallel 
> for' construct is a simple and effective default choice, which is what 
> happens today. 
> target_teams_loop => target_teams_distribute_parallel_for
> Applying the if clause to the parallel region for this case can potentially 
> limit it to one thread, which would hinder performance gains otherwise 
> possible, and presumably wouldn't be what the user wanted/expected.
> 
> The semantics of the spec (OpenMP 5.2 [3.4, if Clause, Semantics, 15-18]) is 
> definitely what should be applied to an explicit instance of 
> target_teams_distribute_parallel_for, but in this case (when mapping 
> target_teams_loop => target_teams_distribute_parallel_for) it seems 
> reasonable to make the choice described above.
I'm not sure this is true users  expectations.


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

https://reviews.llvm.org/D157197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1570-1575
+  // If we are here with a 'target teams loop' then we are emitting the
+  // 'parallel' region of the 'target teams distribute parallel for'
+  // emitted in place of the 'target teams loop'. Based on the specification
+  // noted above, an if-clause associated with a 'target teams loop', be it
+  // 'if(val)' or an 'if(target:val)', will apply only to 'target' and not
+  // the 'parallel' of the 'target teams distribute parallel for'.

ddpagan wrote:
> ABataev wrote:
> > It does not match the spec. 
> > ```
> > For a combined or composite construct, if no directive-name-modifier is 
> > specified then the if clause applies to all constituent constructs to which 
> > an if clause can apply.
> > ```
> > So, if(val) should be applied to both target and parallel regions, no?
> > It does not match the spec. 
> > ```
> > For a combined or composite construct, if no directive-name-modifier is 
> > specified then the if clause applies to all constituent constructs to which 
> > an if clause can apply.
> > ```
> > So, if(val) should be applied to both target and parallel regions, no?
> 
> Hi Alexey - Question for you: does revising the comment above at lines 
> 1570-1575 to the following text help explain in a better way what's being 
> done, and why?
> 
>   If we are handling a 'target teams distribute parallel for' explicitly 
> written
>   in the source, and it has an 'if(val)' clause, the if condition is applied 
> to
>   both 'target' and 'parallel' regions according to
>   OpenMP 5.2 [3.4, if Clause, Semantics, 15-18].
> 
>   However, if we are mapping an explicit 'target teams loop if(val)' onto a
>   'target teams distribute parallel for if(val)', to preserve the 'if' 
> semantics
>   as specified by the user with the 'target teams loop', we apply it just to
>   the 'target' region.
It does not match the spec. Why we shall handle it this way?


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

https://reviews.llvm.org/D157197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123235: [OpenMP] atomic compare fail : Parser & AST support

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/OpenMPClause.h:2320-2321
+: public OMPClause,
+  private llvm::TrailingObjects {
+  OMPClause *MemoryOrderClause;

Why need tail-allocation here for constant number of attributes? They can be 
represented simply as data members



Comment at: clang/include/clang/AST/OpenMPClause.h:2322
+OpenMPClauseKind> {
+  OMPClause *MemoryOrderClause;
+

`OMPClause *MemoryOrderClause = nullptr;`



Comment at: clang/include/clang/AST/OpenMPClause.h:2361
+  : OMPClause(llvm::omp::OMPC_fail, StartLoc, EndLoc) {
+MemoryOrderClause = 0x0;
+  }

Remove this



Comment at: clang/include/clang/AST/OpenMPClause.h:2367
+  : OMPClause(llvm::omp::OMPC_fail, SourceLocation(), SourceLocation()) {
+MemoryOrderClause = 0x0;
+  }

Remove this



Comment at: clang/lib/Basic/OpenMPKinds.cpp:446
+  return "seq_cst";
+default:
+  return "unknown";

Not recommended to use defaul switch, use all enums explicitly



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3306
   case OMPC_compare:
+  case OMPC_fail:
   case OMPC_seq_cst:

Looks like this is the wrong place for this clause, better to parse like  
OMPC_default clause



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3793
+
+  OMPFailClause *FailClause = static_cast(Clause);
+  SourceLocation LParenLoc;

auto *FailClause = cast(Clause)



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12614
+
+class OpenMPAtomicFailChecker {
+

Needs class description



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12615-12616
+class OpenMPAtomicFailChecker {
+
+protected:
+  Sema &SemaRef;

Why protected?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12621-12622
+public:
+  // Error descriptor type which will be returned to Sema
+  unsigned int ErrorNo;
+

Should be private member with the default initializer



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12641
+  for (const OMPClause *C : Clauses) {
+if (const auto *FC = dyn_cast(C)) {
+  NoOfFails++;

```
const auto *FC = dyn_cast(C)
if (!FC)
  continue;
```
to reduce structural complexity



Comment at: clang/lib/Sema/SemaOpenMP.cpp:12657
+break;
+  default: break;
+  }

1. default: cases are not repcommended
2. formatting



Comment at: clang/lib/Serialization/ASTReader.cpp:10572-10580
+  case llvm::omp::OMPC_acquire:
+MemoryOrderClause = new (Context) OMPAcquireClause(SourceLoc, EndLoc);
+break;
+  case llvm::omp::OMPC_relaxed:
+MemoryOrderClause = new (Context) OMPRelaxedClause(SourceLoc, EndLoc);
+break;
+  case llvm::omp::OMPC_seq_cst:

This is strange you need this



Comment at: clang/lib/Serialization/ASTReader.cpp:10581-10582
+break;
+  default:
+break;
+  }

do not use default:



Comment at: clang/lib/Serialization/ASTWriter.cpp:6553-6554
+void OMPClauseWriter::VisitOMPFailClause(OMPFailClause *C) {
+  // Record.AddSourceLocation(C->getLParenLoc());
+  // Copied from VisitOMPUpdateClause
+  Record.AddSourceLocation(C->getLParenLoc());

Remove this


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

https://reviews.llvm.org/D123235

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_parallel_for_simd_tl_codegen.cpp:4
+
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=51 -emit-llvm %s -o - | FileCheck 
--check-prefix=OMP51 %s
+

Add PCH serialization/desrialization checks in your tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1570-1575
+  // If we are here with a 'target teams loop' then we are emitting the
+  // 'parallel' region of the 'target teams distribute parallel for'
+  // emitted in place of the 'target teams loop'. Based on the specification
+  // noted above, an if-clause associated with a 'target teams loop', be it
+  // 'if(val)' or an 'if(target:val)', will apply only to 'target' and not
+  // the 'parallel' of the 'target teams distribute parallel for'.

It does not match the spec. 
```
For a combined or composite construct, if no directive-name-modifier is 
specified then the if clause applies to all constituent constructs to which an 
if clause can apply.
```
So, if(val) should be applied to both target and parallel regions, no?


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

https://reviews.llvm.org/D157197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Please, use the script to generate the checks for the newly added tests


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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157229: Fix edge case in declare target initializer expression

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1564
+  // Any if-clause associated with expansion for 'target teams loop' should
+  // apply to target region only. OpenMP 5.2 [3.4, if Clause, Semantics, 15-18]
+  if (S.getDirectiveKind() != OMPD_target_teams_loop)

I don't see this statement in the standard


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

https://reviews.llvm.org/D157197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Add the reference to openmp spec


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157197

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157140: [clang][Sema][OpenMP] Fix capture region for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:15410
   break;
+case OMPD_target_teams_loop:
+  // This is equivalent to 'target teams distribute parallel for' with

Need to have capture region for parallel, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157140

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/StmtOpenMP.h:291
+  /// the frontend.
+  OpenMPDirectiveKind PrevMappedDirective = llvm::omp::OMPD_unknown;
+

koops wrote:
> ABataev wrote:
> > I don't see where this field is stored/loaded for PCH support. You need add 
> > a support for this in Serialization.
> Since the ASTReader and ASTWriter are used by the developer of clang, it is 
> preferable to show the internal state of the compiler as is, e.g. if "#pragma 
> omp loop bind(parallel)" is changed to "#pragma omp for" then the ASTWriter 
> has to show it as "#pragma omp for". I can change it to "#pragma omp loop 
> bind(parallel)" using the MappedDirective that is stored however, that would 
> be misleading to the developer of clang.
How it is related to PCH? Try to serialize/deserialize the template and then 
try to create a new instance. There will be an issue since there is no 
serialization support, when you try to create new instance in the Rebuild... 
function (mapLoopConstruct will use OMPD_unknown as Prev value instead of 
oringal previous value, since it is not serialized/deserialized)


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/StmtOpenMP.h:291
+  /// the frontend.
+  OpenMPDirectiveKind PrevMappedDirective = llvm::omp::OMPD_unknown;
+

I don't see where this field is stored/loaded for PCH support. You need add a 
support for this in Serialization.



Comment at: clang/include/clang/AST/StmtOpenMP.h:611-614
+  void setMappedDirective(OpenMPDirectiveKind MappedDirective) {
+PrevMappedDirective = MappedDirective;
+  }
+

Better to make it a part of Create member function rather than having a 
separate function for this.


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:1652
+  Stmt *AStmt, SourceLocation StartLoc, SourceLocation EndLoc,
+  OpenMPDirectiveKind prevMappedDirective = OMPD_unknown) {
+

PrevMappedDirective


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/AST/StmtOpenMP.h:291
+  /// the frontend.
+  OpenMPDirectiveKind prevMappedDirective;
+

OpenMPDirectiveKind PrevMappedDirective = llvm::omp::OMPD_unknown;



Comment at: clang/include/clang/AST/StmtOpenMP.h:308
+EndLoc(std::move(EndLoc)),
+prevMappedDirective(llvm::omp::OMPD_unknown) {}
 

Remove initialization of prevMappedDirective with the default value from the 
constructor



Comment at: clang/include/clang/AST/StmtOpenMP.h:616
+
+  OpenMPDirectiveKind getMappedDirective() { return prevMappedDirective; }
+

const member function



Comment at: clang/include/clang/Sema/Sema.h:11476
   StmtResult ActOnOpenMPRegionEnd(StmtResult S, ArrayRef Clauses);
   StmtResult ActOnOpenMPExecutableDirective(
   OpenMPDirectiveKind Kind, const DeclarationNameInfo &DirName,

All these changed functions are called with the default value of this new 
prevMappedDirective paramer. Remove.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6391
 Res = ActOnOpenMPSimdDirective(ClausesWithImplicit, AStmt, StartLoc, 
EndLoc,
-   VarsWithInheritedDSA);
+   VarsWithInheritedDSA, prevMappedDirective);
 if (LangOpts.OpenMP >= 50)

Can you just use DSAStack->getMappedDirective() instead of prevMappedDirective?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:10544
 
-  setFunctionHasBranchProtectedScope();
-  return OMPForDirective::Create(
+  OMPForDirective *ForDirective = OMPForDirective::Create(
   Context, StartLoc, EndLoc, NestedLoopCount, Clauses, AStmt, B,

`auto *ForDirective`


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156352: [OpenMP][Sema] Fix directive name modifier/if-clause/'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9866
+  (CGM.getLangOpts().OpenMP >= 51 &&
+   needsTaskBasedThreadLimit(D.getDirectiveKind()) &&
+   D.hasClausesOfKind());

I think you don't need needsTaskBasedThreadLimit call here, the emitTargetCall 
function itself can be called only for target-based directives



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5143
+if (CGF.CGM.getLangOpts().OpenMP >= 51 &&
+needsTaskBasedThreadLimit(S.getDirectiveKind()) && TL) {
+  // Emit __kmpc_set_thread_limit() to set the thread_limit for the task

Same regarding needsTaskBasedThreadLimit(S.getDirectiveKind()) , the function 
EmitOMPTargetTaskBasedDirective is called only for target-based directives


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155635: [OpenMP] [Reduction] Allow PLUS (+) operator on reduction clauses in OMP > 52

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


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

https://reviews.llvm.org/D155635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155849: [NFC][clang] Fix static analyzer concerns

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155849

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155692: [clang][OpenMP] Add interop support for multiple depend clauses

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155692

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155692: [clang][OpenMP] Add interop support for multiple depend clauses

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:6986
+DependenceAddress = DependenciesArray.getPointer();
+CodeGenFunction::RunCleanupsScope LocalScope(*this);
   }

mhalk wrote:
> Not sure if this is useful here.
> Saw this `RunCleanupsScope` in conjunction with the emission of dependences + 
> calls.
> 
> But, at least in the generated IR it won't remove duplicate `depend` clauses.
> 
> If anybody can comment on this, that would be great.
> Otherwise, I'd tend to remove it.
Not required here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155692

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155635: [OpenMP] [Reduction] Allow PLUS (+) operator on reduction clauses in OMP > 52

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D155635#4512188 , @mdfazlay wrote:

> In D155635#4512183 , @ABataev wrote:
>
>> Tests?
>
> I think I need to use `-fopenmp-version=60` to test this revision. As OMP 6.0 
> is not official yet, am I allowed to use `-fopenmp-version=60` in my LIT test?

Yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155635: [OpenMP] [Reduction] Allow PLUS (+) operator on reduction clauses in OMP > 52

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155635

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:343
+  /// This may also be used in a similar way for other constructs.
+  OpenMPDirectiveKind MappedDirective = OMPD_unknown;
+

ABataev wrote:
> I assume it shall be a member of SharingMapType, otherwise there might be 
> problems with handling of enclosed constructs.
Not done


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9837
+def err_omp_bind_required_on_loop : Error<
+  "expected 'bind' clause for loop construct without an enclosing OpenMP "
+  "construct">;

Also, `'loop'`



Comment at: clang/include/clang/Sema/Sema.h:11136
+  /// rigorous semantic checking in the new mapped directives.
+  bool mapLoopConstruct(llvm::SmallVector &ClausesWithoutBind,
+ArrayRef Clauses,

`llvm::SmallVectorImpl &ClausesWithoutBind`


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9838
+def err_omp_loop_reduction_clause : Error<
+  "reduction clause not allowed with '#pragma omp loop bind(teams)'">;
 def warn_break_binds_to_switch : Warning<

'reduction'



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6105
+bool Sema::mapLoopConstruct(
+llvm::SmallVector *ClausesWithoutBind,
+ArrayRef Clauses, OpenMPBindClauseKind BindKind,

Why ClausesWithoutBind passing by pointer? Use `llvm::SmallVectorImpl &ClausesWithoutBind` instead



Comment at: clang/lib/Sema/SemaOpenMP.cpp:6107
+ArrayRef Clauses, OpenMPBindClauseKind BindKind,
+OpenMPDirectiveKind *Kind) {
+

Same, pass by reference



Comment at: clang/lib/Sema/SemaOpenMP.cpp:14088-14089
   setFunctionHasBranchProtectedScope();
-  return OMPDistributeDirective::Create(Context, StartLoc, EndLoc,
-NestedLoopCount, Clauses, AStmt, B);
+  return OMPDistributeDirective::Create(
+  Context, StartLoc, EndLoc, NestedLoopCount, Clauses, AStmt, B);
 }

Restore formatting


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154672: [OPENMP52] Deprecation of 'depend' clause in ordered directive.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1532
   InGroup;
+def warn_omp_depend_in_ordered_deprecated : Warning<"denpend clause for 
ordered is deprecated; use doacross instaed">, InGroup;
 

denpend->depend
instaed->instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154672

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154556: [OPENMP52] Support Support omp_cur_iteration modifier for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154556: [OPENMP52] Support Support omp_cur_iteration modifier for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/AST/OpenMPClause.cpp:2516-2523
+  if (DepType == OMPC_DOACROSS_source)
+OS << "source:";
+  else if (DepType == OMPC_DOACROSS_sink)
+OS << "sink:";
+  else if (DepType == OMPC_DOACROSS_source_omp_cur_iteration)
+OS << "source: omp_cur_iteration";
+  else if (DepType == OMPC_DOACROSS_sink_omp_cur_iteration)

switch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: clang/lib/CodeGen/CGDecl.cpp:19
 #include "CGOpenMPRuntime.h"
+#include "CGOpenMPRuntimeGPU.h"
 #include "CodeGenFunction.h"

You can remove this include


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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:591
+  auto &RT =
+  *(static_cast(&CGF.CGM.getOpenMPRuntime()));
+  RT.getKmpcFreeShared(CGF, AddrSizePair);

Same, just CGOpenMPRuntime &RT = CGM.getOpenMPRuntime();



Comment at: clang/lib/CodeGen/CGDecl.cpp:1605
+if (getLangOpts().OpenMPIsDevice) {
+  auto &RT = static_cast(CGM.getOpenMPRuntime());
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

Here and in other places, jusy remove the cast to CGOpenMPRuntimeGPU, 
CGM.getOpenMPRuntime() already provides virtual functions, use them directly 
without cast:
```
CGOpenMPRuntime &RT = CGM.getOpenMPRuntime();
```



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:699-710
+  /// Get call to __kmpc_alloc_shared
+  virtual std::pair
+  getKmpcAllocShared(CodeGenFunction &CGF, const VarDecl *VD) {
+llvm_unreachable("not implemented");
+  }
+
+  /// Get call to __kmpc_free_shared

doru1004 wrote:
> @ABataev I have added the interface entries here.
Then you already good, just do not gast to CGOpenMPRuntimeGPU, use 
CGM.getOpenMPRuntime() directly since it already has these member functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1606
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGM.getOpenMPRuntime()));
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

doru1004 wrote:
> ABataev wrote:
> > ABataev wrote:
> > > 1. use `static_cast(CGM.getOpenMPRuntime())`
> > > 2. It will crash if your device is not GPU. Better to make 
> > > `getKmpcAllocShared` and `getKmpcFreeShared` virtual (just like 
> > > `isDelayedVariableLengthDecl`) in base CGOpenMPRuntime, since it may be 
> > > required not only for GPU-based devices.
> > Check the second item, please, better to make all new member function 
> > virtual and handle it for non-GPU devices too
> The support I am adding is only meant for GPUs. I am not sure why we need to 
> consider non-GPUs. There already exists a VLA handling for non-GPUs and that 
> one should be used.
1. It will crash the compiler if your device is not a GPU (say, CPU).
2. I'm not asking to implement it for non-GPU, I'm asking to provide common 
interface. The general implementation should call just llvm_unreachable, 
nothing else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1606
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGM.getOpenMPRuntime()));
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

ABataev wrote:
> 1. use `static_cast(CGM.getOpenMPRuntime())`
> 2. It will crash if your device is not GPU. Better to make 
> `getKmpcAllocShared` and `getKmpcFreeShared` virtual (just like 
> `isDelayedVariableLengthDecl`) in base CGOpenMPRuntime, since it may be 
> required not only for GPU-based devices.
Check the second item, please, better to make all new member function virtual 
and handle it for non-GPU devices too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154180: [OPENMP52] Codegen support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:2285-2297
+return (C->getDependencyKind() == OMPC_DEPEND_sink);
+  }
+  bool isSource(const OMPDependClause *C) {
+return (C->getDependencyKind() == OMPC_DEPEND_source);
+  }
+};
+template <> class OMPDoacrossKind {

remove parens from the expressions, they are not needed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154180: [OPENMP52] Codegen support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11410
   } else {
-assert(C->getDependencyKind() == OMPC_DEPEND_sink);
+assert(ODK.isSink(C));
 RTLFn = OMPBuilder.getOrCreateRuntimeFunction(CGM.getModule(),

Add a message, please



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:2279
+public:
+  bool isSink(const T *) { return false; }
+};

Add isSOurce 



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5860-5862
+  bool IsDependSource = false;
+  if (ODK.isSource(C))
+IsDependSource = true;

bool IsDependSource = ODK.isSource(C);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1606
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGM.getOpenMPRuntime()));
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

1. use `static_cast(CGM.getOpenMPRuntime())`
2. It will crash if your device is not GPU. Better to make `getKmpcAllocShared` 
and `getKmpcFreeShared` virtual (just like `isDelayedVariableLengthDecl`) in 
base CGOpenMPRuntime, since it may be required not only for GPU-based devices.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:261
+  else
+DelayedVariableLengthDecls.insert(VD);
+} else

Yep, this is what I meant. The only question: do you really need this new 
parameter? CGF.CapturedStmtInfo provides the list of captures and you can try 
to use it.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1100-1104
+// Check if the size of the VLA is available at this point i.e. check that
+// it has been emitted already. If not available then skip it and use
+// delayed emission of __kmpc_alloc_shared.
+if (llvm::is_contained(I->getSecond().DelayedVariableLengthDecls, VD))
+  continue;

Do you still need this check?


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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154180: [OPENMP52] Codegen support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11405-11406
+  llvm::OpenMPIRBuilder &OMPBuilder = CGM.getOpenMPRuntime().getOMPBuilder();
+  const OMPDependClause *DepC = dyn_cast(C);
+  const OMPDoacrossClause *DoC = dyn_cast(C);
+  if ((DoC && DoC->getDependenceType() == OMPC_DOACROSS_source) ||

1. const auto *
2. Can you try torework it to make it more C++-ish? Use template deduction to 
avoid runtime dyn_casts.
e.g. something like:
```
namespace {
template 
class OMPDoacrossKind {
public:
bool isSink(const T *) {return false;}
}
}
template <>
class OMPDoacrossKInd {
public:
bool isSink(const OMPDependClause* C) {return C->getDependenceType();}
}
...
```
and use `if (OMPDoacrossKInd::isSink(C)) RTLFn = ...` and same for `Source`



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5859-5868
+  const auto DOC = dyn_cast(C);
+  const auto DC = dyn_cast(C);
+  bool IsDependSource = false;
+  if ((DC && DC->getDependencyKind() == OMPC_DEPEND_source) ||
+  (DOC && DOC->getDependenceType() == OMPC_DOACROSS_source))
+IsDependSource = true;
+  CGF.Builder.restoreIP(

Same, use template-based analysis instead of runtime-based.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2806
+  /// Return true if all the emissions for the VLA size have occured.
+  bool hasVLASize(const VariableArrayType *type);
+

doru1004 wrote:
> doru1004 wrote:
> > ABataev wrote:
> > > doru1004 wrote:
> > > > doru1004 wrote:
> > > > > doru1004 wrote:
> > > > > > ABataev wrote:
> > > > > > > doru1004 wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > doru1004 wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > doru1004 wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > 1. Is it possible that VariableArrayType does not 
> > > > > > > > > > > > > have VLA size?
> > > > > > > > > > > > > 2. Fix param name
> > > > > > > > > > > > @ABataev How would point 1 happen?
> > > > > > > > > > > You're adding a function that checks if VLA type has VLA 
> > > > > > > > > > > size. I'm asking, if it is possible for VLA type to not 
> > > > > > > > > > > have VLA size at all? Why do you need this function?
> > > > > > > > > > This function checks if the expression of the size of the 
> > > > > > > > > > VLA has already been emitted and can be used.
> > > > > > > > > Why the emission of VLA size can be delayed?
> > > > > > > > Because the size of the VLA is emitted in the user code and the 
> > > > > > > > prolog of the function happens before that. The emission of the 
> > > > > > > > VLA needs to be delayed until its size has been emitted in the 
> > > > > > > > user code.
> > > > > > > This is very fragile approach. Can you try instead try to improve 
> > > > > > > markAsEscaped function and fix insertion of VD to 
> > > > > > > EscapedVariableLengthDecls and if the declaration is internal for 
> > > > > > > the target region, insert it to DelayedVariableLengthDecls?
> > > > > > I am not sure what the condition would be, at that point, to choose 
> > > > > > between one list or the other. I'm not sure what you mean by the 
> > > > > > declaration being internal to the target region.
> > > > > Any thoughts? As far as I can tell all VLAs that reach that point 
> > > > > belong in `DelayedVariableLengthDecls`
> > > > @ABataev I cannot think of a condition to use for the distinction in 
> > > > markedAsEscaped(). Could you please explain in more detail what you 
> > > > want me to check? I can make the rest of the changes happen no problem 
> > > > but I don't know what the condition is. Unless you tell me otherwise, I 
> > > > think the best condition is to check whether the VLA size has been 
> > > > emitted (i.e. that is is part of the VLASize list) in which case the 
> > > > code as is now is fine.
> > > Can you check that the declaration is not captured in the target context? 
> > > If it is not captured, it is declared in the target region and should be 
> > > emitted as delayed.
> > How do I check that? There doesn't seem to be a list of captured variables 
> > available at that point in the code.
> > 
> So the complication is that the same declaration is captured and not captured 
> at the same time. It can be declared inside a teams distribute (not captured) 
> but captured by an inner parallel for (captured). I think I can come up with 
> something though.
Need to check the captures in the target regions only


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2806
+  /// Return true if all the emissions for the VLA size have occured.
+  bool hasVLASize(const VariableArrayType *type);
+

doru1004 wrote:
> doru1004 wrote:
> > doru1004 wrote:
> > > ABataev wrote:
> > > > doru1004 wrote:
> > > > > ABataev wrote:
> > > > > > doru1004 wrote:
> > > > > > > ABataev wrote:
> > > > > > > > doru1004 wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > 1. Is it possible that VariableArrayType does not have VLA 
> > > > > > > > > > size?
> > > > > > > > > > 2. Fix param name
> > > > > > > > > @ABataev How would point 1 happen?
> > > > > > > > You're adding a function that checks if VLA type has VLA size. 
> > > > > > > > I'm asking, if it is possible for VLA type to not have VLA size 
> > > > > > > > at all? Why do you need this function?
> > > > > > > This function checks if the expression of the size of the VLA has 
> > > > > > > already been emitted and can be used.
> > > > > > Why the emission of VLA size can be delayed?
> > > > > Because the size of the VLA is emitted in the user code and the 
> > > > > prolog of the function happens before that. The emission of the VLA 
> > > > > needs to be delayed until its size has been emitted in the user code.
> > > > This is very fragile approach. Can you try instead try to improve 
> > > > markAsEscaped function and fix insertion of VD to 
> > > > EscapedVariableLengthDecls and if the declaration is internal for the 
> > > > target region, insert it to DelayedVariableLengthDecls?
> > > I am not sure what the condition would be, at that point, to choose 
> > > between one list or the other. I'm not sure what you mean by the 
> > > declaration being internal to the target region.
> > Any thoughts? As far as I can tell all VLAs that reach that point belong in 
> > `DelayedVariableLengthDecls`
> @ABataev I cannot think of a condition to use for the distinction in 
> markedAsEscaped(). Could you please explain in more detail what you want me 
> to check? I can make the rest of the changes happen no problem but I don't 
> know what the condition is. Unless you tell me otherwise, I think the best 
> condition is to check whether the VLA size has been emitted (i.e. that is is 
> part of the VLASize list) in which case the code as is now is fine.
Can you check that the declaration is not captured in the target context? If it 
is not captured, it is declared in the target region and should be emitted as 
delayed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154180: [OPENMP52] Codegen support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11401-11402
   llvm::Value *Args[] = {
-  emitUpdateLocation(CGF, C->getBeginLoc()),
-  getThreadID(CGF, C->getBeginLoc()),
+  RT.emitUpdateLocation(CGF, C->getBeginLoc()),
+  RT.getThreadID(CGF, C->getBeginLoc()),
   CGF.Builder.CreateConstArrayGEP(CntAddr, 0).getPointer()};

RT is needed only for loc and tid, you can preemit them in the member functions 
and pass here instead of emitting them here and expose private interfaces.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11422-11428
+  const OMPDependClause *CL) {
+  return EmitDoacrossOrdered(*this, CGF, CGM, CL);
+}
+
+void CGOpenMPRuntime::emitDoacrossOrdered(CodeGenFunction &CGF,
+  const OMPDoacrossClause *CL) {
+  return EmitDoacrossOrdered(*this, CGF, CGM, CL);

Use Cl or just C instead of CL.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1650-1660
+
+  /// Gets thread id value for the current thread.
+  ///
+  llvm::Value *getThreadID(CodeGenFunction &CGF, SourceLocation Loc);
+
+  /// Emits object of ident_t type with info for source location.
+  /// \param Flags Flags for OpenMP location.

Why these declarations are moved?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2806
+  /// Return true if all the emissions for the VLA size have occured.
+  bool hasVLASize(const VariableArrayType *type);
+

doru1004 wrote:
> ABataev wrote:
> > doru1004 wrote:
> > > ABataev wrote:
> > > > doru1004 wrote:
> > > > > ABataev wrote:
> > > > > > 1. Is it possible that VariableArrayType does not have VLA size?
> > > > > > 2. Fix param name
> > > > > @ABataev How would point 1 happen?
> > > > You're adding a function that checks if VLA type has VLA size. I'm 
> > > > asking, if it is possible for VLA type to not have VLA size at all? Why 
> > > > do you need this function?
> > > This function checks if the expression of the size of the VLA has already 
> > > been emitted and can be used.
> > Why the emission of VLA size can be delayed?
> Because the size of the VLA is emitted in the user code and the prolog of the 
> function happens before that. The emission of the VLA needs to be delayed 
> until its size has been emitted in the user code.
This is very fragile approach. Can you try instead try to improve markAsEscaped 
function and fix insertion of VD to EscapedVariableLengthDecls and if the 
declaration is internal for the target region, insert it to 
DelayedVariableLengthDecls?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2168
+ElementType = VLAType->getElementType();
+llvm::Value *VLASize = VLASizeMap[VLAType->getSizeExpr()];
+if (!VLASize)

Use VLASizeMap.find() instead



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2806
+  /// Return true if all the emissions for the VLA size have occured.
+  bool hasVLASize(const VariableArrayType *type);
+

doru1004 wrote:
> ABataev wrote:
> > doru1004 wrote:
> > > ABataev wrote:
> > > > 1. Is it possible that VariableArrayType does not have VLA size?
> > > > 2. Fix param name
> > > @ABataev How would point 1 happen?
> > You're adding a function that checks if VLA type has VLA size. I'm asking, 
> > if it is possible for VLA type to not have VLA size at all? Why do you need 
> > this function?
> This function checks if the expression of the size of the VLA has already 
> been emitted and can be used.
Why the emission of VLA size can be delayed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2806
+  /// Return true if all the emissions for the VLA size have occured.
+  bool hasVLASize(const VariableArrayType *type);
+

doru1004 wrote:
> ABataev wrote:
> > 1. Is it possible that VariableArrayType does not have VLA size?
> > 2. Fix param name
> @ABataev How would point 1 happen?
You're adding a function that checks if VLA type has VLA size. I'm asking, if 
it is possible for VLA type to not have VLA size at all? Why do you need this 
function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:590-591
+void Emit(CodeGenFunction &CGF, Flags EmissionFlags) override {
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGF.CGM.getOpenMPRuntime()));
+  RT.getKmpcFreeShared(CGF, AddrSizePair);

```
auto &RT = static_cast(...);
```



Comment at: clang/lib/CodeGen/CGDecl.cpp:1605-1606
+if (getLangOpts().OpenMPIsDevice) {
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGM.getOpenMPRuntime()));
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

No need to cast to CGOpenMPRuntimeGPU since isDelayedVariableLengthDecl is a 
member of CGOpenMPRuntime.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1116-1120
+  for (const auto *DelayedD : I->getSecond().DelayedVariableLengthDecls)
+if (DelayedD == VD)
+  return true;
+
+  return false;

```
return llvm::is_contained(I->getSecond().DelayedVariableLengthDecls, VD);
```



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1152
+CodeGenFunction &CGF,
+std::pair AddrSizePair) {
+  // Deallocate the memory for each globalized VLA object

pass it here and in other places as const reference



Comment at: clang/lib/CodeGen/CodeGenFunction.h:2806
+  /// Return true if all the emissions for the VLA size have occured.
+  bool hasVLASize(const VariableArrayType *type);
+

1. Is it possible that VariableArrayType does not have VLA size?
2. Fix param name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1605-1609
+(CGM.getContext().getTargetInfo().getTriple().isAMDGPU() ||
+ CGM.getContext().getTargetInfo().getTriple().isNVPTX())) {
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGM.getOpenMPRuntime()));
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

doru1004 wrote:
> ABataev wrote:
> > I think you can drop triple checks and rely completely on 
> > RT.isDelayedVariableLengthDecl(*this, &D) result here
> I tried it but there is a lit test (which I cannot identify) that hangs when 
> offloading to the host (I think) so it has to be an actual GPU. Any ideas?
Make isDelayedVariableLengthDecl virtual in base OpenMPRuntime and make it 
return false by default, and true in base implementation for GPU. This should 
fix the problem, I hope


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154180: [OPENMP52] Codegen support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11382-11383
 
 void CGOpenMPRuntime::emitDoacrossOrdered(CodeGenFunction &CGF,
-  const OMPDependClause *C) {
+  const OMPClause *CL) {
+  const OMPDependClause *DC =

I suggest to create template static function, specialized for `OMPDependClause` 
and `OMPDoacrossClause`to avoid all those multiple selects.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11383
 void CGOpenMPRuntime::emitDoacrossOrdered(CodeGenFunction &CGF,
-  const OMPDependClause *C) {
+  const OMPClause *CL) {
+  const OMPDependClause *DC =

Just `C` is better, or `Cl`



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11384-11387
+  const OMPDependClause *DC =
+  isa(CL) ? dyn_cast(CL) : nullptr;
+  const OMPDoacrossClause *DoC =
+  isa(CL) ? dyn_cast(CL) : nullptr;

```
const auto *DepC = dyn_cast(CL);
const auto *DoC = dyn_cast(CL);
```



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11410-11429
+  if (DoC) {
+if (DoC->getDependenceType() == OMPC_DOACROSS_source) {
+  RTLFn = OMPBuilder.getOrCreateRuntimeFunction(
+  CGM.getModule(), OMPRTL___kmpc_doacross_post);
+} else {
+  assert(DoC->getDependenceType() == OMPC_DOACROSS_sink);
+  RTLFn = OMPBuilder.getOrCreateRuntimeFunction(

Try to unify this code, avoid duplications



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5845-5846
+  llvm::OpenMPIRBuilder &OMPBuilder) {
+  auto DOC = dyn_cast(C);
+  auto DC = dyn_cast(C);
+

const auto *



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5874-5889
 if (S.hasClausesOfKind()) {
   // The ordered directive with depend clause.
   assert(!S.hasAssociatedStmt() &&
  "No associated statement must be in ordered depend construct.");
   InsertPointTy AllocaIP(AllocaInsertPt->getParent(),
  AllocaInsertPt->getIterator());
+  for (const auto *DC : S.getClausesOfKind())

Same, try to unify the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154180

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Delay emission of __kmpc_alloc_shared for escaped VLAs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:589
+: AddrSizePair(AddrSizePair) {}
+void Emit(CodeGenFunction &CGF, Flags Flags) override {
+  CGOpenMPRuntimeGPU &RT =

Name of the variable hides the type, potential warning or even error



Comment at: clang/lib/CodeGen/CGDecl.cpp:1605-1609
+(CGM.getContext().getTargetInfo().getTriple().isAMDGPU() ||
+ CGM.getContext().getTargetInfo().getTriple().isNVPTX())) {
+  CGOpenMPRuntimeGPU &RT =
+  *(static_cast(&CGM.getOpenMPRuntime()));
+  if (RT.isDelayedVariableLengthDecl(*this, &D)) {

I think you can drop triple checks and rely completely on 
RT.isDelayedVariableLengthDecl(*this, &D) result here



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2164-2174
+bool CodeGenFunction::hasVLASize(const VariableArrayType *type) {
+  QualType elementType;
+  do {
+elementType = type->getElementType();
+llvm::Value *vlaSize = VLASizeMap[type->getSizeExpr()];
+if (!vlaSize)
+  return false;

Fix var naming


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153321: [OpenMP] Fix lvalue reference type generation in untied task loop

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG08513cbea4dc: [OpenMP] Fix lvalue reference type generation 
in untied task loop (authored by eastb233, committed by ABataev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153321

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/taskloop_untied_codegen.cpp


Index: clang/test/OpenMP/taskloop_untied_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/taskloop_untied_codegen.cpp
@@ -0,0 +1,26 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py 
UTC_ARGS: --version 2
+// RUN: %clang_cc1 -verify -triple aarch64-unknown-linux-gnu -fopenmp -x c++ 
-std=c++11 -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+// CHECK-LABEL: define dso_local void @_Z15taskloop_untiedv
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[WORK:%.*]] = alloca [100 x float], align 4
+// CHECK-NEXT:[[AGG_CAPTURED:%.*]] = alloca [[STRUCT_ANON:%.*]], align 1
+// CHECK-NEXT:[[TMP0:%.*]] = call i32 @__kmpc_global_thread_num(ptr 
@[[GLOB1:[0-9]+]])
+// CHECK-NEXT:[[TMP1:%.*]] = call ptr @__kmpc_omp_task_alloc(ptr 
@[[GLOB1]], i32 [[TMP0]], i32 0, i64 472, i64 1, ptr @.omp_task_entry.)
+// CHECK-NEXT:[[TMP2:%.*]] = getelementptr inbounds 
[[STRUCT_KMP_TASK_T_WITH_PRIVATES:%.*]], ptr [[TMP1]], i32 0, i32 0
+// CHECK-NEXT:[[TMP3:%.*]] = getelementptr inbounds 
[[STRUCT_KMP_TASK_T_WITH_PRIVATES]], ptr [[TMP1]], i32 0, i32 1
+// CHECK-NEXT:[[TMP4:%.*]] = getelementptr inbounds 
[[STRUCT__KMP_PRIVATES_T:%.*]], ptr [[TMP3]], i32 0, i32 3
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[TMP4]], ptr 
align 4 [[WORK]], i64 400, i1 false)
+// CHECK-NEXT:[[TMP5:%.*]] = getelementptr inbounds 
[[STRUCT_KMP_TASK_T:%.*]], ptr [[TMP2]], i32 0, i32 2
+// CHECK-NEXT:store i32 0, ptr [[TMP5]], align 8
+// CHECK-NEXT:[[TMP6:%.*]] = call i32 @__kmpc_omp_task(ptr @[[GLOB1]], i32 
[[TMP0]], ptr [[TMP1]])
+// CHECK-NEXT:ret void
+//
+void taskloop_untied() {
+  float work[100];
+#pragma omp task untied
+  for (auto cb : work)
+cb = 1.0;
+}
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -4852,6 +4852,8 @@
   // a pointer to this memory.
   for (auto &Pair : UntiedLocalVars) {
 QualType VDType = Pair.first->getType().getNonReferenceType();
+if (Pair.first->getType()->isLValueReferenceType())
+  VDType = CGF.getContext().getPointerType(VDType);
 if (isAllocatableDecl(Pair.first)) {
   llvm::Value *Ptr = CGF.Builder.CreateLoad(Pair.second.first);
   Address Replacement(


Index: clang/test/OpenMP/taskloop_untied_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/taskloop_untied_codegen.cpp
@@ -0,0 +1,26 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
+// RUN: %clang_cc1 -verify -triple aarch64-unknown-linux-gnu -fopenmp -x c++ -std=c++11 -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+// CHECK-LABEL: define dso_local void @_Z15taskloop_untiedv
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[WORK:%.*]] = alloca [100 x float], align 4
+// CHECK-NEXT:[[AGG_CAPTURED:%.*]] = alloca [[STRUCT_ANON:%.*]], align 1
+// CHECK-NEXT:[[TMP0:%.*]] = call i32 @__kmpc_global_thread_num(ptr @[[GLOB1:[0-9]+]])
+// CHECK-NEXT:[[TMP1:%.*]] = call ptr @__kmpc_omp_task_alloc(ptr @[[GLOB1]], i32 [[TMP0]], i32 0, i64 472, i64 1, ptr @.omp_task_entry.)
+// CHECK-NEXT:[[TMP2:%.*]] = getelementptr inbounds [[STRUCT_KMP_TASK_T_WITH_PRIVATES:%.*]], ptr [[TMP1]], i32 0, i32 0
+// CHECK-NEXT:[[TMP3:%.*]] = getelementptr inbounds [[STRUCT_KMP_TASK_T_WITH_PRIVATES]], ptr [[TMP1]], i32 0, i32 1
+// CHECK-NEXT:[[TMP4:%.*]] = getelementptr inbounds [[STRUCT__KMP_PRIVATES_T:%.*]], ptr [[TMP3]], i32 0, i32 3
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i64(ptr align 8 [[TMP4]], ptr align 4 [[WORK]], i64 400, i1 false)
+// CHECK-NEXT:[[TMP5:%.*]] = getelementptr inbounds [[STRUCT_KMP_TASK_T:%.*]], ptr [[TMP2]], i32 0, i32 2
+// CHECK-NEXT:store i32 0, ptr [[TMP5]], align 8
+// CHECK-NEXT:[[TMP6:%.*]] = call i32 @__kmpc_omp_task(ptr @[[GLOB1]], i32 [[TMP0]], ptr [[TMP1]])
+// CHECK-NEXT:ret void
+//
+void taskloop_untied() {
+  float work[100];
+#pragma omp task untied
+  for (auto cb : work)
+cb = 1.0;
+}
Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -4852,6 +485

[PATCH] D153556: [OPENMP52] Initial support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


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

https://reviews.llvm.org/D153556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153556: [OPENMP52] Initial support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:20871
   TotalDepCount.getZExtValue());
+
   if ((DepKind == OMPC_DEPEND_sink || DepKind == OMPC_DEPEND_source) &&

Remove this new line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153556: [OPENMP52] Initial support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Upload full context


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Enable use of __kmpc_alloc_shared for VLAs defined in AMD GPU offloaded regions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D153883#4456342 , @tianshilei1992 
wrote:

> I think it's better to just limit it to AMDGPU for now.

I rather doubt this is a good decision. Better to support for all targets. 
NVPTX supports(ed) (IIRC) static allocation and internal management for the 
shared memory (not sure it is true for the new library). If no, then we need at 
least to diagnose that this feature is not supported.

> BTW, it might be worth to check if heap-to-stack will push it back to stack.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153321: [OpenMP] Fix lvalue reference type generation in untied task loop

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153321

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153556: [OPENMP52] Initial support for doacross clause.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:4415-4439
+  } else if (Kind == OMPC_doacross) {
+// Handle dependence type for the doacross clause.
+ColonProtectionRAIIObject ColonRAII(*this);
+Data.ExtraModifier = getOpenMPSimpleClauseType(
+Kind, Tok.is(tok::identifier) ? PP.getSpelling(Tok) : "",
+getLangOpts());
+Data.ExtraModifierLoc = Tok.getLocation();

jyu2 wrote:
> ABataev wrote:
> > Can it be unified with depenbd clause parsing? (Maybe in a separate 
> > template function)
> I don't really has an idea on how to combine this two with template function. 
>  Since depend clause in ordered is deprecated in 52, and will be removed, 
> should we leave as this?
Even ff it will be removed in 52, it will still stay for OpenMP < 52. Would be 
good to try to unify it.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:20694-20700
+auto *C = OMPDoacrossClause::Create(
+Context, StartLoc, LParenLoc, EndLoc,
+IsSource ? OMPC_DOACROSS_source : OMPC_DOACROSS_sink, DepLoc, ColonLoc,
+Vars, TotalDepCount.getZExtValue());
+if (DSAStack->isParentOrderedRegion())
+  DSAStack->addDoacrossDependClause(C, OpsOffs);
+return C;

Better to create clauses in ActOnDoAcross and ActOnDepend, this function better 
to return required data as a struct/class/bolean, etc.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:20701
+return C;
+  } else {
+auto *C = OMPDependClause::Create(

No need for else


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

https://reviews.llvm.org/D153556

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9818
+  D.hasClausesOfKind() ||
+  (CGM.getLangOpts().OpenMP >= 51 && D.getDirectiveKind() == OMPD_target &&
+   D.hasClausesOfKind());

What if D is combined target directive, i.e. D.getDirectiveKind() is something 
like OMPD_target_teams, etc.?



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5143-5148
+S.getSingleClause()) {
+  // Emit __kmpc_set_thread_limit() to set the thread_limit for the task
+  // enclosing this target region. This will indirectly set the 
thread_limit
+  // for every applicable construct within target region.
+  CGF.CGM.getOpenMPRuntime().emitThreadLimitClause(
+  CGF, S.getSingleClause()->getThreadLimit(),

Avoid double call of S.getSingleClause(), store in local 
variable call result.



Comment at: clang/test/OpenMP/target_codegen.cpp:849
 // OMP51: [[CE:%.*]] = load {{.*}} [[CEA]]
-// OMP51: call i32 @__tgt_target_kernel({{.*}}, i64 -1, i32 -1, i32 [[CE]],
+// OMP51: call ptr @__kmpc_omp_task_alloc({{.*@.omp_task_entry.*}})
+// OMP51: call i32 [[OMP_TASK_ENTRY]]

It requires extra resource consumption, can you try to avoid creating outer 
task, if possible?



Comment at: openmp/runtime/src/kmp_ftn_entry.h:809
+return thread_limit;
+  else
+return thread->th.th_current_task->td_icvs.thread_limit;

No need for else here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Enable use of __kmpc_alloc_shared for VLAs defined in AMD GPU offloaded regions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1603
+// deallocation call of __kmpc_free_shared() is emitted later.
+if (getLangOpts().OpenMP && getTarget().getTriple().isAMDGCN()) {
+  // Emit call to __kmpc_alloc_shared() instead of the alloca.

doru1004 wrote:
> jhuber6 wrote:
> > ABataev wrote:
> > > OpenMPIsDevice?
> > Does NVPTX handle this already? If not, is there a compelling reason to 
> > exclude NVPTX? Otherwise we should check if we are the OpenMP device.
> Does NVPTX support dynamic allocas?
It does not matter here, it depends on the runtime library implementations. The 
compiler just shall provide proper runtime calls emission, everything else is 
part of the runtime support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Enable use of __kmpc_alloc_shared for VLAs defined in AMD GPU offloaded regions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1085
   }
-  for (const auto *VD : I->getSecond().EscapedVariableLengthDecls) {
-// Use actual memory size of the VLA object including the padding

jhuber6 wrote:
> doru1004 wrote:
> > ABataev wrote:
> > > Why this code is removed?
> > I could not understand why this code is here in the first place since it 
> > doesn't seem that it could ever work correctly (and it doesn't seem to be 
> > covered by any existing tests). Maybe I'm wrong but that was my 
> > understanding of it. So what seems to happen is that this code attempts to 
> > emit a kmpc_alloc_shared before the actual size calculation is emitted. So 
> > if the VLA size is something that the user defines such as `int N = 10;` 
> > then that code will not have been emitted at this point. When the 
> > expression computing the size of the VLA uses `N`, the code that is deleted 
> > here would just fail to find the VLA size in the attempt to emit the 
> > kmpc_alloc_shared. The emission of the VLA as kmpc_alloc_shared needs to 
> > happen after the expression of the size is emitted.
> I'm pretty sure I was the one that wrote this code, and at the time I don't 
> recall it really working. I remember there was something else that expected 
> this to be here, but for what utility I do not recall. VLAs were never tested 
> or used.
They are tested, check 
test/OpenMP/nvptx_target_teams_distribute_parallel_for_codegen.cpp for example, 
where it captures VLA implicitly. I assume this should not be AMDGCN specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153883: [Clang][OpenMP] Enable use of __kmpc_alloc_shared for VLAs defined in AMD GPU offloaded regions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Add the runtime test?




Comment at: clang/lib/CodeGen/CGDecl.cpp:587
+std::pair AddrSizePair;
+KmpcAllocFree(std::pair AddrSizePair)
+: AddrSizePair(AddrSizePair) {}

Better to pass it as const reference



Comment at: clang/lib/CodeGen/CGDecl.cpp:589
+: AddrSizePair(AddrSizePair) {}
+void Emit(CodeGenFunction &CGF, Flags flags) override {
+  CGOpenMPRuntimeGPU &RT =

Wrong param name, use Camel



Comment at: clang/lib/CodeGen/CGDecl.cpp:1603
+// deallocation call of __kmpc_free_shared() is emitted later.
+if (getLangOpts().OpenMP && getTarget().getTriple().isAMDGCN()) {
+  // Emit call to __kmpc_alloc_shared() instead of the alloca.

OpenMPIsDevice?



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1085
   }
-  for (const auto *VD : I->getSecond().EscapedVariableLengthDecls) {
-// Use actual memory size of the VLA object including the padding

Why this code is removed?



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1112
+
+  return std::pair({VoidPtr, Size});
+}

Use `std::make_pair(VoidPtr, Size)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153883

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153369: [OpenMP] Always apply target declarations to canonical definitions

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: clang/lib/Sema/SemaOpenMP.cpp:22991
 ML->DeclarationMarkedOpenMPDeclareTarget(ND, A);
+
   checkDeclIsAllowedInOpenMPTarget(nullptr, ND, Loc);

Remove new line here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153369

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:11094
 
+  bool checkLastPrivateForMappedDirectives(ArrayRef Clauses);
+  bool mapLoopConstruct(llvm::SmallVector *ClausesWithoutBind,

const member function?
Add a comment



Comment at: clang/include/clang/Sema/Sema.h:11095
+  bool checkLastPrivateForMappedDirectives(ArrayRef Clauses);
+  bool mapLoopConstruct(llvm::SmallVector *ClausesWithoutBind,
+ArrayRef Clauses,

Add a comment



Comment at: clang/include/clang/Sema/Sema.h:11095
+  bool checkLastPrivateForMappedDirectives(ArrayRef Clauses);
+  bool mapLoopConstruct(llvm::SmallVector *ClausesWithoutBind,
+ArrayRef Clauses,

ABataev wrote:
> Add a comment
`SmallVectorImpl &` instead of pointer to fully specified 
`SmallVector`


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

https://reviews.llvm.org/D144634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits