[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83261#2224619 , @cchen wrote:

> In PR45212 comment 5 (https://bugs.llvm.org/show_bug.cgi?id=45212#c5), there 
> is a reduced test case that failed after adding this patch. The assertion is 
> from OMPChildren::getInnermostCapturedStmt.
>
> Test case:
>
>   #include 
>   #include 
>   #include 
>   
>   class Myclass
>   {
> public:
> double A[10];
> double B[10];
>   
> void add(int i)
> {
>   for (int k = 0; k < 10; k++) {
>   #pragma omp atomic
> B[k] += A[i];
>   }
> }
>   };
>   
>   int main(int argc, char* argv[]){
> Myclass foo;
>
> for (int i = 0; i < 10; i++) {
> foo.A[i] = i;
> foo.B[i] = 0;
> }
>   
>   #pragma omp target teams distribute parallel for
> for (int i = 0; i < 10; i++) {
> foo.add(i);
> }
>   
> printf("Correctness check: B[2]= %f \n",foo.B[2]);
> return 0;
>   }
>
> Backtrace:
>
>   Assertion failed: (isa(Val) && "cast() argument of incompatible 
> type!"), function cast, file 
> /Users/cchen/workspace/llvm-project/llvm/include/llvm/Support/Casting.h, line 
> 269.
>   PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
> backtrace, preprocessed source, and associated run script.
>   Stack dump:
>   0.  Program arguments: /Users/cchen/llvm/bin/clang-10 -cc1 -triple nvptx64 
> -aux-triple x86_64-apple-darwin18.7.0 -Wundef-prefix=TARGET_OS_ 
> -Werror=undef-prefix -Wdeprecated-objc-isa-usage 
> -Werror=deprecated-objc-isa-usage -emit-llvm -disable-free -main-file-name 
> test.cpp -mrelocation-model static -mframe-pointer=all -fno-rounding-math 
> -fno-verbose-asm -no-integrated-as 
> -fcompatibility-qualified-id-block-type-checking -target-cpu sm_35 
> -fno-split-dwarf-inlining -debugger-tuning=gdb -target-linker-version 512.4 
> -resource-dir /Users/cchen/llvm/lib/clang/12.0.0 -internal-isystem 
> /Users/cchen/llvm/lib/clang/12.0.0/include/openmp_wrappers -include 
> __clang_openmp_device_functions.h -internal-isystem 
> /Users/cchen/llvm/bin/../include/c++/v1 -internal-isystem /usr/include/c++/v1 
> -internal-isystem /Users/cchen/llvm/bin/../include/c++/v1 -internal-isystem 
> /usr/include/c++/v1 -internal-isystem /usr/local/include -internal-isystem 
> /Users/cchen/llvm/lib/clang/12.0.0/include -internal-externc-isystem 
> /usr/include -internal-isystem /usr/local/include -internal-isystem 
> /Users/cchen/llvm/lib/clang/12.0.0/include -internal-externc-isystem 
> /usr/include -fdeprecated-macro -fno-dwarf-directory-asm 
> -fdebug-compilation-dir /Users/cchen/workspace/llvm-project/bugfix/45212 
> -ferror-limit 19 -fopenmp -fopenmp-version=50 
> -fopenmp-cuda-parallel-target-regions -fgnuc-version=4.2.1 -fcxx-exceptions 
> -fexceptions -fcolor-diagnostics -fopenmp-is-device 
> -fopenmp-host-ir-file-path 
> /var/folders/rx/8phxx7k53pqghv8kjxkbv2ch006fcl/T/test-c282ef.bc -o 
> /var/folders/rx/8phxx7k53pqghv8kjxkbv2ch006fcl/T/test-f0824e.ll -x c++ 
> test.cpp
>   1.  test.cpp:20:1: current parser token 'int'
>   2.  test.cpp:5:7: LLVM IR generation of declaration 'Myclass'
>   0  clang-10 0x000108bead4c 
> llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
>   1  clang-10 0x000108beb309 
> PrintStackTraceSignalHandler(void*) + 25
>   2  clang-10 0x000108be8c46 
> llvm::sys::RunSignalHandlers() + 118
>   3  clang-10 0x000108bed070 SignalHandler(int) + 208
>   4  libsystem_platform.dylib 0x7fff7bd98b5d _sigtramp + 29
>   5  clang-10 0x00010fdc6ae2 
> llvm::DenseMapInfo::Tombstone + 2998754
>   6  libsystem_c.dylib0x7fff7bc526a6 abort + 127
>   7  libsystem_c.dylib0x7fff7bc1b20d basename_r + 0
>   8  clang-10 0x00010e908344 
> llvm::cast_retty::ret_type 
> llvm::cast(clang::Stmt*) + 100
>   9  clang-10 0x00010d4097b3 
> clang::OMPChildren::getInnermostCapturedStmt(llvm::ArrayRef)
>  + 227
>   10 clang-10 0x00010d3f67d5 
> clang::OMPExecutableDirective::getInnermostCapturedStmt() + 197
>   11 clang-10 0x00010959a555 
> clang::OMPExecutableDirective::getInnermostCapturedStmt() const + 21
>   12 clang-10 0x000109599d89 
> clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
> const*, llvm::StringRef) + 1305
>   13 clang-10 0x000109599eab 
> clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
> const*, llvm::StringRef) + 1595
>   14 clang-10 0x000109599eab 
> clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
> const*, llvm::StringRef) + 1595
>   15 clang-10 0x000109599eab 
> clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
> const*, llvm::StringRef) + 1595
>   16 

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

In PR45212 comment 5 (https://bugs.llvm.org/show_bug.cgi?id=45212#c5), there is 
a reduced test case that failed after adding this patch. The assertion is from 
OMPChildren::getInnermostCapturedStmt.

Test case:

  #include 
  #include 
  #include 
  
  class Myclass
  {
public:
  double A[10];
  double B[10];
  
void add(int i)
{
  for (int k = 0; k < 10; k++) {
  #pragma omp atomic
B[k] += A[i];
  }
}
  };
  
  int main(int argc, char* argv[]){
Myclass foo;
   
for (int i = 0; i < 10; i++) {
foo.A[i] = i;
foo.B[i] = 0;
}
  
  #pragma omp target teams distribute parallel for
for (int i = 0; i < 10; i++) {
foo.add(i);
}
  
printf("Correctness check: B[2]= %f \n",foo.B[2]);
return 0;
  }

Backtrace:

  Assertion failed: (isa(Val) && "cast() argument of incompatible 
type!"), function cast, file 
/Users/cchen/workspace/llvm-project/llvm/include/llvm/Support/Casting.h, line 
269.
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.Program arguments: /Users/cchen/llvm/bin/clang-10 -cc1 -triple nvptx64 
-aux-triple x86_64-apple-darwin18.7.0 -Wundef-prefix=TARGET_OS_ 
-Werror=undef-prefix -Wdeprecated-objc-isa-usage 
-Werror=deprecated-objc-isa-usage -emit-llvm -disable-free -main-file-name 
test.cpp -mrelocation-model static -mframe-pointer=all -fno-rounding-math 
-fno-verbose-asm -no-integrated-as 
-fcompatibility-qualified-id-block-type-checking -target-cpu sm_35 
-fno-split-dwarf-inlining -debugger-tuning=gdb -target-linker-version 512.4 
-resource-dir /Users/cchen/llvm/lib/clang/12.0.0 -internal-isystem 
/Users/cchen/llvm/lib/clang/12.0.0/include/openmp_wrappers -include 
__clang_openmp_device_functions.h -internal-isystem 
/Users/cchen/llvm/bin/../include/c++/v1 -internal-isystem /usr/include/c++/v1 
-internal-isystem /Users/cchen/llvm/bin/../include/c++/v1 -internal-isystem 
/usr/include/c++/v1 -internal-isystem /usr/local/include -internal-isystem 
/Users/cchen/llvm/lib/clang/12.0.0/include -internal-externc-isystem 
/usr/include -internal-isystem /usr/local/include -internal-isystem 
/Users/cchen/llvm/lib/clang/12.0.0/include -internal-externc-isystem 
/usr/include -fdeprecated-macro -fno-dwarf-directory-asm 
-fdebug-compilation-dir /Users/cchen/workspace/llvm-project/bugfix/45212 
-ferror-limit 19 -fopenmp -fopenmp-version=50 
-fopenmp-cuda-parallel-target-regions -fgnuc-version=4.2.1 -fcxx-exceptions 
-fexceptions -fcolor-diagnostics -fopenmp-is-device -fopenmp-host-ir-file-path 
/var/folders/rx/8phxx7k53pqghv8kjxkbv2ch006fcl/T/test-c282ef.bc -o 
/var/folders/rx/8phxx7k53pqghv8kjxkbv2ch006fcl/T/test-f0824e.ll -x c++ test.cpp
  1.test.cpp:20:1: current parser token 'int'
  2.test.cpp:5:7: LLVM IR generation of declaration 'Myclass'
  0  clang-10 0x000108bead4c 
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
  1  clang-10 0x000108beb309 
PrintStackTraceSignalHandler(void*) + 25
  2  clang-10 0x000108be8c46 llvm::sys::RunSignalHandlers() 
+ 118
  3  clang-10 0x000108bed070 SignalHandler(int) + 208
  4  libsystem_platform.dylib 0x7fff7bd98b5d _sigtramp + 29
  5  clang-10 0x00010fdc6ae2 
llvm::DenseMapInfo::Tombstone + 2998754
  6  libsystem_c.dylib0x7fff7bc526a6 abort + 127
  7  libsystem_c.dylib0x7fff7bc1b20d basename_r + 0
  8  clang-10 0x00010e908344 
llvm::cast_retty::ret_type 
llvm::cast(clang::Stmt*) + 100
  9  clang-10 0x00010d4097b3 
clang::OMPChildren::getInnermostCapturedStmt(llvm::ArrayRef)
 + 227
  10 clang-10 0x00010d3f67d5 
clang::OMPExecutableDirective::getInnermostCapturedStmt() + 197
  11 clang-10 0x00010959a555 
clang::OMPExecutableDirective::getInnermostCapturedStmt() const + 21
  12 clang-10 0x000109599d89 
clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
const*, llvm::StringRef) + 1305
  13 clang-10 0x000109599eab 
clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
const*, llvm::StringRef) + 1595
  14 clang-10 0x000109599eab 
clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
const*, llvm::StringRef) + 1595
  15 clang-10 0x000109599eab 
clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt 
const*, llvm::StringRef) + 1595
  16 clang-10 0x00010959a6f0 
clang::CodeGen::CGOpenMPRuntime::emitTargetFunctions(clang::GlobalDecl) + 320
  17 clang-10 0x00010959b97e 
clang::CodeGen::CGOpenMPRuntime::emitTargetGlobal(clang::GlobalDecl) + 142
  18 clang-10 0x00010973c53e 

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3329
   }
-  OMPDeclareMapperDecl *NewDMD = 
SemaRef.ActOnOpenMPDeclareMapperDirectiveStart(
-  /*S=*/nullptr, Owner, D->getDeclName(), SubstMapperTy, D->getLocation(),

Meinersbur wrote:
> The refactoring of ActOnOpenMPDeclareMapperDirective seems unrelated.
These changes are required by the redesign of the declare mapper representation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261

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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

I think the ActOnOpenMPDeclareMapperDirective changes should be separated into 
its own patch. Otherwise, LGTM.




Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:3329
   }
-  OMPDeclareMapperDecl *NewDMD = 
SemaRef.ActOnOpenMPDeclareMapperDirectiveStart(
-  /*S=*/nullptr, Owner, D->getDeclName(), SubstMapperTy, D->getLocation(),

The refactoring of ActOnOpenMPDeclareMapperDirective seems unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261

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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:345
 Diag(Loc, diag::err_omp_declare_mapper_wrong_var)
-<< DMD->getVarName().getAsString();
+<< getOpenMPDeclareMapperVarName();
 Diag(D->getLocation(), diag::note_entity_declared_at) << D;

I'd like to point out that in general there is no need to pass a string to a 
diagnostic instead of a `NamedDecl *`. Doing so will bypass the customisation 
points `NamedDecl::getNameForDiagnostic` and `NamedDecl::printName`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261

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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83261#2166766 , @Meinersbur wrote:

> In D83261#2162561 , @ABataev wrote:
>
> > 1. OMPChildren class uses standard TrailingObjects harness instead of 
> > manual calculation.
>
>
> Note that that having a separate object defeats the purpose of 
> `TrailingObjects` of having just a single allocation per AST node. If we do 
> separate objects, we could also have member pointers to arrays.


I know. Will check what I can do about it.

> 
> 
> In D83261#2164929 , @ABataev wrote:
> 
>> Sure, we can make `OMPChildren` common for declarative and executable 
>> directives. Do you want me to do it?
> 
> 
> Yes, I think it would increase its usefulness and remove code duplication of 
> handling clauses.
> 
 There should be an additional patch, which, I hope, should simplify things 
 for loop-based directives.
>>> 
>>> OK. What does this patch do? Are you going to upload it as well?
>> 
>> At first, need to deal with this one, at least come to an agreement with the 
>> design.
> 
> The reviewer list is surprisingly small. Aren't there any others with stakes 
> in the class hierarchy?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261



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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D83261#2162561 , @ABataev wrote:

> 1. OMPChildren class uses standard TrailingObjects harness instead of manual 
> calculation.


Note that that having a separate object defeats the purpose of 
`TrailingObjects` of having just a single allocation per AST node. If we do 
separate objects, we could also have member pointers to arrays.

In D83261#2164929 , @ABataev wrote:

> Sure, we can make `OMPChildren` common for declarative and executable 
> directives. Do you want me to do it?


Yes, I think it would increase its usefulness and remove code duplication of 
handling clauses.

>>> There should be an additional patch, which, I hope, should simplify things 
>>> for loop-based directives.
>> 
>> OK. What does this patch do? Are you going to upload it as well?
> 
> At first, need to deal with this one, at least come to an agreement with the 
> design.

The reviewer list is surprisingly small. Aren't there any others with stakes in 
the class hierarchy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261



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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83261#2162904 , @Meinersbur wrote:

> In D83261#2162561 , @ABataev wrote:
>
> > 1. OMPChildren class uses standard TrailingObjects harness instead of 
> > manual calculation.
>
>
> I was going to argue that OMPExecutableDirective could derive from 
> TrailingObjects as well, but it requires a template parameter for its final 
> subclass. Good point!
>
> > 2. Child Stmt* based nodes are not related to the AsssociatedStmt* anymore 
> > and can exist independently.
>
> It moved into the OMPChildren class. Not sure whether it is better.


What I mean, that previously child nodes could not exist without 
AssociatedStmt, i.e. you have to have AssociatedStmt to have other children. It 
is solved (though, of course, can be implemented in a different way with the 
current design).

> 
> 
>>> OMPChildren also handles clauses for OMPExecutableDirectives but not for 
>>> declarative directives. Should handling of of clauses also be extracted 
>>> into into its own class? That would make (de-)serialization easier for 
>>> those as well.
>> 
>> This class is only for executable directives.
> 
> Nearly all directives have clauses. At the moment they all implement their 
> own clause list. I don't see why clause management should be centralized for 
> executable statements only.

Sure, we can make `OMPChildren` common for declarative and executable 
directives. Do you want me to do it?

> 
> 
>>> There is no effect on D76342  (except a 
>>> requiring a rebase), since the OMPTileDirective has children and thus does 
>>> not profit from the functionality of `OMPChildren` becoming optional. Since 
>>> I don't see a relation to D76342 , more , 
>>> I am indifferent to whether this should be merged.
>> 
>> There should be an additional patch, which, I hope, should simplify things 
>> for loop-based directives.
> 
> OK. What does this patch do? Are you going to upload it as well?

At first, need to deal with this one, at least come to an agreement with the 
design.

> 
> 
>>> Trailing objects is a technique to ensure that all substmts are consecutive 
>>> in memory (so `StmtIterator` can iterator over them). For 
>>> OMPExeuctableDirectives, only the associated statement is returned by the 
>>> `StmtIterator`, i.e. all the children could be made regular class members 
>>> without the complication of computing the offset. I'd prefer that change 
>>> over OMPChildren.
>> 
>> There are also used_children, which are used by the clang analyzer for, at 
>> least, use of uninitialized variables diagnostics.
> 
> OK, I see.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261



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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D83261#2162561 , @ABataev wrote:

> 1. OMPChildren class uses standard TrailingObjects harness instead of manual 
> calculation.


I was going to argue that OMPExecutableDirective could derive from 
TrailingObjects as well, but it requires a template parameter for its final 
subclass. Good point!

> 2. Child Stmt* based nodes are not related to the AsssociatedStmt* anymore 
> and can exist independently.

It moved into the OMPChildren class. Not sure whether it is better.

>> OMPChildren also handles clauses for OMPExecutableDirectives but not for 
>> declarative directives. Should handling of of clauses also be extracted into 
>> into its own class? That would make (de-)serialization easier for those as 
>> well.
> 
> This class is only for executable directives.

Nearly all directives have clauses. At the moment they all implement their own 
clause list. I don't see why clause management should be centralized for 
executable statements only.

>> There is no effect on D76342  (except a 
>> requiring a rebase), since the OMPTileDirective has children and thus does 
>> not profit from the functionality of `OMPChildren` becoming optional. Since 
>> I don't see a relation to D76342 , more , I 
>> am indifferent to whether this should be merged.
> 
> There should be an additional patch, which, I hope, should simplify things 
> for loop-based directives.

OK. What does this patch do? Are you going to upload it as well?

>> Trailing objects is a technique to ensure that all substmts are consecutive 
>> in memory (so `StmtIterator` can iterator over them). For 
>> OMPExeuctableDirectives, only the associated statement is returned by the 
>> `StmtIterator`, i.e. all the children could be made regular class members 
>> without the complication of computing the offset. I'd prefer that change 
>> over OMPChildren.
> 
> There are also used_children, which are used by the clang analyzer for, at 
> least, use of uninitialized variables diagnostics.

OK, I see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261



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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83261#2161217 , @Meinersbur wrote:

> AFAICS this extract out the handling of subnodes of OMPExecutableDirectives 
> into the OMPChildren class which is made optional since `OMPChildren 
> *OMPExecutableDirectives::Data` can be nullptr. However, since it also stores 
> clauses, it seems that about every executable directive will need to have one 
> anyway. Hence I don't see how it makes the representation of some executable 
> directives more correct.




1. OMPChildren class uses standard TrailingObjects harness instead of manual 
calculation.
2. Child Stmt* based nodes are not related to the AsssociatedStmt* anymore and 
can exist independently.

> OMPChildren also handles clauses for OMPExecutableDirectives but not for 
> declarative directives. Should handling of of clauses also be extracted into 
> into its own class? That would make (de-)serialization easier for those as 
> well.

This class is only for executable directives.

> There is no effect on D76342  (except a 
> requiring a rebase), since the OMPTileDirective has children and thus does 
> not profit from the functionality of `OMPChildren` becoming optional. Since I 
> don't see a relation to D76342 , more , I am 
> indifferent to whether this should be merged.

There should be an additional patch, which, I hope, should simplify things for 
loop-based directives.

> Trailing objects is a technique to ensure that all substmts are consecutive 
> in memory (so `StmtIterator` can iterator over them). For 
> OMPExeuctableDirectives, only the associated statement is returned by the 
> `StmtIterator`, i.e. all the children could be made regular class members 
> without the complication of computing the offset. I'd prefer that change over 
> OMPChildren.

There are also used_children, which are used by the clang analyzer for, at 
least, use of uninitialized variables diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261



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

[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

AFAICS this extract out the handling of subnodes of OMPExecutableDirectives 
into the OMPChildren class which is made optional since `OMPChildren 
*OMPExecutableDirectives::Data` can be nullptr. However, since it also stores 
clauses, it seems that about every executable directive will need to have one 
anyway. Hence I don't see how it makes the representation of some executable 
directives more correct.

OMPChildren also handles clauses for OMPExecutableDirectives but not for 
declarative directives. Should handling of of clauses also be extracted into 
into its own class? That would make (de-)serialization easier for those as well.

There is no effect on D76342  (except a 
requiring a rebase), since the OMPTileDirective has children and thus does not 
profit from the functionality of `OMPChildren` becoming optional. Since I don't 
see a relation to D76342 , more , I am 
indifferent to whether this should be merged.

Trailing objects is a technique to ensure that all substmts are consecutive in 
memory (so `StmtIterator` can iterator over them). For OMPExeuctableDirectives, 
only the associated statement is returned by the `StmtIterator`, i.e. all the 
children could be made regular class members without the complication of 
computing the offset. I'd prefer that change over OMPChildren.




Comment at: clang/include/clang/AST/StmtOpenMP.h:147
+
+  Stmt *getRawStmt() {
+assert(HasAssociatedStmt &&

This looks like the equivalent to `ignoreCaptures` from D76342. Could you 
document it what it does differently from 
IgnoreContainers/getCapturedStmt/getInnermostCapturedStmt/getBody/getStructuredBlock?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261



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