Introduced OMPChildren class to handle all associated clauses, statement
and child expressions/statements. It allows to represent some directives
more correctly (like flush, depobj etc. with pseudo clauses, ordered
depend directives, which are standalone, and target data directives).
ALso, it will make easier to avoid using of CapturedStmt in directives,
if required (atomic, tile etc. directives).
Also, it simplifies serialization/deserialization of the executable
directives.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
clang/include/clang/AST/StmtOpenMP.h | ||
---|---|---|
147 | This looks like the equivalent to ignoreCaptures from D76342. Could you document it what it does differently from IgnoreContainers/getCapturedStmt/getInnermostCapturedStmt/getBody/getStructuredBlock? |
- OMPChildren class uses standard TrailingObjects harness instead of manual calculation.
- 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.
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!
- 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.
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.
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.
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?
I know. Will check what I can do about 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?
- Added OMPChildren to the declartive directive to simplify their representation.
- Reduce memory fragmentation for declare mapper.
- Removed previously introduced memory fragmentation.
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
345 ↗ | (On Diff #282649) | 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. |
I think the ActOnOpenMPDeclareMapperDirective changes should be separated into its own patch. Otherwise, LGTM.
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
---|---|---|
3329 ↗ | (On Diff #282677) | The refactoring of ActOnOpenMPDeclareMapperDirective seems unrelated. |
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
---|---|---|
3329 ↗ | (On Diff #282677) | These changes are required by the redesign of the declare mapper representation. |
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 <cmath> #include <omp.h> #include <iostream> 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<X>(Val) && "cast<Ty>() 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 0x0000000108bead4c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60 1 clang-10 0x0000000108beb309 PrintStackTraceSignalHandler(void*) + 25 2 clang-10 0x0000000108be8c46 llvm::sys::RunSignalHandlers() + 118 3 clang-10 0x0000000108bed070 SignalHandler(int) + 208 4 libsystem_platform.dylib 0x00007fff7bd98b5d _sigtramp + 29 5 clang-10 0x000000010fdc6ae2 llvm::DenseMapInfo<llvm::codeview::GloballyHashedType>::Tombstone + 2998754 6 libsystem_c.dylib 0x00007fff7bc526a6 abort + 127 7 libsystem_c.dylib 0x00007fff7bc1b20d basename_r + 0 8 clang-10 0x000000010e908344 llvm::cast_retty<clang::CapturedStmt, clang::Stmt*>::ret_type llvm::cast<clang::CapturedStmt, clang::Stmt>(clang::Stmt*) + 100 9 clang-10 0x000000010d4097b3 clang::OMPChildren::getInnermostCapturedStmt(llvm::ArrayRef<llvm::omp::Directive>) + 227 10 clang-10 0x000000010d3f67d5 clang::OMPExecutableDirective::getInnermostCapturedStmt() + 197 11 clang-10 0x000000010959a555 clang::OMPExecutableDirective::getInnermostCapturedStmt() const + 21 12 clang-10 0x0000000109599d89 clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt const*, llvm::StringRef) + 1305 13 clang-10 0x0000000109599eab clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt const*, llvm::StringRef) + 1595 14 clang-10 0x0000000109599eab clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt const*, llvm::StringRef) + 1595 15 clang-10 0x0000000109599eab clang::CodeGen::CGOpenMPRuntime::scanForTargetRegionsFunctions(clang::Stmt const*, llvm::StringRef) + 1595 16 clang-10 0x000000010959a6f0 clang::CodeGen::CGOpenMPRuntime::emitTargetFunctions(clang::GlobalDecl) + 320 17 clang-10 0x000000010959b97e clang::CodeGen::CGOpenMPRuntime::emitTargetGlobal(clang::GlobalDecl) + 142 18 clang-10 0x000000010973c53e clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) + 926 19 clang-10 0x0000000109746a8a clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) + 266 20 clang-10 0x0000000109937553 (anonymous namespace)::CodeGeneratorImpl::EmitDeferredDecls() + 179 21 clang-10 0x000000010993748d (anonymous namespace)::CodeGeneratorImpl::HandlingTopLevelDeclRAII::~HandlingTopLevelDeclRAII() + 77 22 clang-10 0x00000001099373f5 (anonymous namespace)::CodeGeneratorImpl::HandlingTopLevelDeclRAII::~HandlingTopLevelDeclRAII() + 21 23 clang-10 0x0000000109932410 (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) + 176 24 clang-10 0x00000001096f5d6e clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) + 222 25 clang-10 0x000000010ca8cd14 clang::ParseAST(clang::Sema&, bool, bool) + 548 26 clang-10 0x0000000109e76e82 clang::ASTFrontendAction::ExecuteAction() + 322 27 clang-10 0x00000001096f4f17 clang::CodeGenAction::ExecuteAction() + 2391 28 clang-10 0x0000000109e76411 clang::FrontendAction::Execute() + 129 29 clang-10 0x0000000109dbd367 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 2087 30 clang-10 0x0000000109fce131 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 1761 31 clang-10 0x00000001063404ec cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 1388 32 clang-10 0x0000000106333a02 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) + 434 33 clang-10 0x00000001063326d5 main + 1509 34 libdyld.dylib 0x00007fff7bbad3d5 start + 1
Edit: Removing "#pragma omp atomic" in line 14 can avoid the asserts.
clang-format suggested style edits found: