[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57615#1381461 , @ABataev wrote:

> In D57615#1381443 , @lebedev.ri 
> wrote:
>
> > Let's instead solve the problem of structured block not having an AST 
> > representation.
>
>
> Again, this is not a problem for the compiler. If you want this idiom for the 
> analyzer, you need to teach the analyzer to handle the AST nodes in 
> accordance with the OpenMP standard.


I'm sorry, but //to me// that just sounds completely wrong.

It sounds remotely like the gcc argument of keeping everything in
the least modular fashion just so no one does anything bad with it.

OpenMP spec gives a pretty good idea when something is an 'structured block'.
I'm specifically looking for these OpenMP structured blocks.
Are you telling me that i should forget everything i know about the 'goals' of 
clang AST,
(and i do believe it is reasonable to expect to have an `OMPStructuredBlock` 
opaque entry)
and reimplement the 'OpenMP structured blocks' finding in whatever code i'm 
writing,
instead of doing the right thing, and teaching the AST about it?

Obviously, said teaching should not have any negative impact on the existing 
clang OpenMP code.
Obviously, everyone's time is limited, and i won't be able to convince anyone 
to do that //for// me :)

I can see just how **quickly** i have derailed this disscussion, in less than 
12h.
I guess i should stop commenting, and follow "words are cheap, show the code".


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D57615#1381467 , @lebedev.ri wrote:

> In D57615#1381447 , @ABataev wrote:
>
> > > Okay, enough is enough :)
> > >  I think a very important phrase was repeated a number of times, and it 
> > > highlights the **actual** problem here:
> > > 
> > >> is not the representation of the structured block
> >
> > It is not a problem! It is the internal implementation design! What do you 
> > want to get? Why do you need all these flags?
>
>
> Uhm, i did state that in the commit message of rL352882 
>  :
>
> >Summary:
> >   I'm working on a clang-tidy check, much like existing [[ 
> > http://clang.llvm.org/extra/clang-tidy/checks/bugprone-exception-escape.html
> >  | bugprone-exception-escape ]],
> >   to detect when an exception might escape out of an OpenMP construct it 
> > isn't supposed to escape from.
>
> and in https://bugs.llvm.org/show_bug.cgi?id=40563#c4 :
>
> > (In reply to Roman Lebedev from comment #4)
> >  > (In reply to Alexey Bataev from comment #3)
> >  > > (In reply to Roman Lebedev from comment #2)
> >  > > > (In reply to Alexey Bataev from comment #1)
> >  > > > CapturedDecl is not the representation of the structured block. It 
> > used just
> >  > > > to simplify the parsing/sema/codegen. The outlined function is not 
> > generated
> >  > > > for the loop, so there is no problem with the standard compatibility.
> >  > > 
> >  > > Exactly. Perhaps i have poorly titled the bug.
> >  > > 
> >  > > The real question was formulated in the last phrase:
> >  > > 
> >  > > > There needs to be some way to represent the fact that
> >  > > > only the body is nothrow.
> >  > 
> >  > What for? It does not help with anything. Standard does not require to
> >  > perform a thorough analysis of the structured blocks for the single
> >  > entry/single exit criteria. It just states that such OpenMP directives 
> > are
> >  > not compatible with the standard. What do you want to get with this?
> > 
> > Such syntactic sugar in AST potentially helps in the same way the very
> > existence
> >  of well-defined AST helps - it is much easier to further operate on an
> > well-defined
> >  AST, rather than on something less specified for that. (IR, asm, ...)
> > 
> > In this case, i'm going through the OP spec, through the every
> >  > A throw executed inside a ??? region must cause execution to resume 
> > within the
> >  > same iteration of the ??? region, and the same thread that threw the 
> > exception must
> >  > catch it.
> >  note, and trying to verify that it is what clang does, either via
> >  CapturedDecl's 'nothrow' tag,
> >  or, like in this case, by filing an issue.
> > 
> > I want this info, so i can use this finely-structured info to do at least
> > partial
> >  validation that these notes "no exception may escape" in the standard are
> >  not violated.
> > 
> > In my case, it will be a clang-tidy check, because there is existing infra
> >  for that.
> >  A clang static analyzer check may appear later, or it may not.
> >  (Though it would be especially interesting in the presence of cross-TU
> >  analysis.)
> > 
> > Therefore i *insist* that it would be //nice// to have this info in the AST
> >  :)
> > 
> > This is not a bug in a form "omg your implementation is so broken, fix it
> >  immediately",
> >  i'm simply using it to track the remaining issues. When i'm done with the
> >  easy cases,
> >  i'll try to cycle back here, and look how this could be solved.
>
> Does that answer the question?
>
> >> Then could you please revoke LG from D57594 
> >> .
> > 
> > Done!
>
> Thanks.


AST already has all the required information. You just need to teach your 
analyzer how to get it. The structured blocks are not represented by 
CapturedStmt/CapturedDecl, in many cases they are hidden inside of them.  You 
just need to get it. But it does not mean that this a compiler problem.
For some constructs, you may have 2,3 or 4 captured statements, but the 
structured block is just 1 and it is hidden somewhere inside for better codegen 
support.
And you need to teach the analyzer how to get this data from the existing AST 
nodes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57615#1381447 , @ABataev wrote:

> > Okay, enough is enough :)
> >  I think a very important phrase was repeated a number of times, and it 
> > highlights the **actual** problem here:
> > 
> >> is not the representation of the structured block
>
> It is not a problem! It is the internal implementation design! What do you 
> want to get? Why do you need all these flags?


Uhm, i did state that in the commit message of rL352882 
 :

>Summary:
>   I'm working on a clang-tidy check, much like existing [[ 
> http://clang.llvm.org/extra/clang-tidy/checks/bugprone-exception-escape.html 
> | bugprone-exception-escape ]],
>   to detect when an exception might escape out of an OpenMP construct it 
> isn't supposed to escape from.

and in https://bugs.llvm.org/show_bug.cgi?id=40563#c4 :

> (In reply to Roman Lebedev from comment #4)
>  > (In reply to Alexey Bataev from comment #3)
>  > > (In reply to Roman Lebedev from comment #2)
>  > > > (In reply to Alexey Bataev from comment #1)
>  > > > CapturedDecl is not the representation of the structured block. It 
> used just
>  > > > to simplify the parsing/sema/codegen. The outlined function is not 
> generated
>  > > > for the loop, so there is no problem with the standard compatibility.
>  > > 
>  > > Exactly. Perhaps i have poorly titled the bug.
>  > > 
>  > > The real question was formulated in the last phrase:
>  > > 
>  > > > There needs to be some way to represent the fact that
>  > > > only the body is nothrow.
>  > 
>  > What for? It does not help with anything. Standard does not require to
>  > perform a thorough analysis of the structured blocks for the single
>  > entry/single exit criteria. It just states that such OpenMP directives are
>  > not compatible with the standard. What do you want to get with this?
> 
> Such syntactic sugar in AST potentially helps in the same way the very
> existence
>  of well-defined AST helps - it is much easier to further operate on an
> well-defined
>  AST, rather than on something less specified for that. (IR, asm, ...)
> 
> In this case, i'm going through the OP spec, through the every
>  > A throw executed inside a ??? region must cause execution to resume within 
> the
>  > same iteration of the ??? region, and the same thread that threw the 
> exception must
>  > catch it.
>  note, and trying to verify that it is what clang does, either via
>  CapturedDecl's 'nothrow' tag,
>  or, like in this case, by filing an issue.
> 
> I want this info, so i can use this finely-structured info to do at least
> partial
>  validation that these notes "no exception may escape" in the standard are
>  not violated.
> 
> In my case, it will be a clang-tidy check, because there is existing infra
>  for that.
>  A clang static analyzer check may appear later, or it may not.
>  (Though it would be especially interesting in the presence of cross-TU
>  analysis.)
> 
> Therefore i *insist* that it would be //nice// to have this info in the AST
>  :)
> 
> This is not a bug in a form "omg your implementation is so broken, fix it
>  immediately",
>  i'm simply using it to track the remaining issues. When i'm done with the
>  easy cases,
>  i'll try to cycle back here, and look how this could be solved.

Does that answer the question?

>> Then could you please revoke LG from D57594 
>> .
> 
> Done!

Thanks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D57615#1381443 , @lebedev.ri wrote:

> Let's instead solve the problem of structured block not having an AST 
> representation.


Again, this is not a problem for the compiler. If you want this idiom for the 
analyzer, you need to teach the analyzer to handle the AST nodes in accordance 
with the OpenMP standard.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

> Okay, enough is enough :)
>  I think a very important phrase was repeated a number of times, and it 
> highlights the **actual** problem here:
> 
>> is not the representation of the structured block

It is not a problem! It is the internal implementation design! What do you want 
to get? Why do you need all these flags?

> Then could you please revoke LG from D57594 .

Done!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri abandoned this revision.
lebedev.ri added a comment.

Let's instead solve the problem of structured block not having an AST 
representation.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57615#1381428 , @ABataev wrote:

> In D57615#1381427 , @lebedev.ri 
> wrote:
>
> > In D57615#1381418 , @ABataev wrote:
> >
> > > In D57615#1381416 , @lebedev.ri 
> > > wrote:
> > >
> > > > @ABataev i'm not sure i have fully followed the 
> > > > https://bugs.llvm.org/show_bug.cgi?id=40563#c1
> > > >
> > > > > The outlined function is not generated for the loop, so there is no 
> > > > > problem with the standard compatibility.
> > > >
> > > > Are you saying that in these cases of `master`, `critical`, `single` 
> > > > directives, `CapturedDecl` should **not** have `nothrow` bit set too?
> > >
> > >
> > > This flag just does not matter for them.
> >
> >
> > I'll rephrase:
> >  Are you opposed to providing the correct (as per the specification) 
> > knowledge that
> >  no exception will escape out of these `CapturedDecl`'s, because that 
> > knowledge
> >  does not matter for the existing sema/codegen, and does not affect 
> > produced IR?
>
>
> Again, CapturedDecl and CapturedStmt is not the representation of the 
> structured block. It is a helper structure for the codegen only. This flag 
> does not help with anything, because for such OpenMP constructs the codegen 
> bypasses CapturedStmt/CapturedDecl.


Okay, enough is enough :)
I think a very important phrase was repeated a number of times, and it 
highlights the **actual** problem here:

> is not the representation of the structured block



> I'm not opposed. I'm saying, that it is not required and not used. If it is 
> not used, why do you want to set it?

Then could you please revoke LG from D57594 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D57615#1381427 , @lebedev.ri wrote:

> In D57615#1381418 , @ABataev wrote:
>
> > In D57615#1381416 , @lebedev.ri 
> > wrote:
> >
> > > @ABataev i'm not sure i have fully followed the 
> > > https://bugs.llvm.org/show_bug.cgi?id=40563#c1
> > >
> > > > The outlined function is not generated for the loop, so there is no 
> > > > problem with the standard compatibility.
> > >
> > > Are you saying that in these cases of `master`, `critical`, `single` 
> > > directives, `CapturedDecl` should **not** have `nothrow` bit set too?
> >
> >
> > This flag just does not matter for them.
>
>
> I'll rephrase:
>  Are you opposed to providing the correct (as per the specification) 
> knowledge that
>  no exception will escape out of these `CapturedDecl`'s, because that 
> knowledge
>  does not matter for the existing sema/codegen, and does not affect produced 
> IR?


Again, CapturedDecl and CapturedStmt is not the representation of the 
structured block. It is a helper structure for the codegen only. This flag does 
not help with anything, because for such OpenMP constructs the codegen bypasses 
CapturedStmt/CapturedDecl.
I'm not opposed. I'm saying, that it is not required and not used. If it is not 
used, why do you want to set it?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D57615#1381418 , @ABataev wrote:

> In D57615#1381416 , @lebedev.ri 
> wrote:
>
> > @ABataev i'm not sure i have fully followed the 
> > https://bugs.llvm.org/show_bug.cgi?id=40563#c1
> >
> > > The outlined function is not generated for the loop, so there is no 
> > > problem with the standard compatibility.
> >
> > Are you saying that in these cases of `master`, `critical`, `single` 
> > directives, `CapturedDecl` should **not** have `nothrow` bit set too?
>
>
> This flag just does not matter for them.


I'll rephrase:
Are you opposed to providing the correct (as per the specification) knowledge 
that
no exception will escape out of these `CapturedDecl`'s, because that knowledge
does not matter for the existing sema/codegen, and does not affect produced IR?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D57615#1381416 , @lebedev.ri wrote:

> @ABataev i'm not sure i have fully followed the 
> https://bugs.llvm.org/show_bug.cgi?id=40563#c1
>
> > The outlined function is not generated for the loop, so there is no problem 
> > with the standard compatibility.
>
> Are you saying that in these cases of `master`, `critical`, `single` 
> directives, `CapturedDecl` should **not** have `nothrow` bit set too?


This flag just does not matter for them.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@ABataev i'm not sure i have fully followed the 
https://bugs.llvm.org/show_bug.cgi?id=40563#c1

> The outlined function is not generated for the loop, so there is no problem 
> with the standard compatibility.

Are you saying that in these cases of `master`, `critical`, `single` 
directives, `CapturedDecl` should **not** have `nothrow` bit set too?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: lib/Sema/SemaOpenMP.cpp:6058
+  // longjmp() and throw() must not violate the entry/exit criteria.
+  cast(AStmt)->getCapturedDecl()->setNothrow();
+

Generally speaking, this is not required. It is required only for those 
constructs, that really needs an outlined function. Master construct does not 
create outlined function, so the "nothrow" flag is useless here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57615



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

[PATCH] D57615: [AST][OpenMP] OpenMP master Construct contains Structured block

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: ABataev, OpenMP.
lebedev.ri added projects: clang, OpenMP.
Herald added subscribers: openmp-commits, guansong.

Much like `single` construct (D57594 ):

`OpenMP Application Programming Interface Version 5.0 November 2018` 
, 
`2.16 master Construct`, starting with page 221:

- The `master` construct specifies a **structured block** that is executed by 
the master thread of the team.
- Restrictions • A throw executed inside a `master` region must cause execution 
to resume within the same `master` region, and the same thread that threw the 
exception must catch it


Repository:
  rC Clang

https://reviews.llvm.org/D57615

Files:
  lib/Sema/SemaOpenMP.cpp
  test/AST/ast-dump-openmp-master.cpp


Index: test/AST/ast-dump-openmp-master.cpp
===
--- /dev/null
+++ test/AST/ast-dump-openmp-master.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -verify -fopenmp -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp-simd -ast-dump %s | FileCheck %s
+// expected-no-diagnostics
+
+void master() {
+#pragma omp master
+  {
+  }
+}
+
+// CHECK: `-FunctionDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-OMPMasterDirective
+// CHECK-NEXT:   `-CapturedStmt
+// CHECK-NEXT: `-CapturedDecl {{.*}} nothrow
+// CHECK-NEXT:   |-CompoundStmt
+// CHECK-NEXT:   `-ImplicitParamDecl
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -6050,6 +6050,13 @@
 
   assert(isa(AStmt) && "Captured statement expected");
 
+  // 1.2.2 OpenMP Language Terminology
+  // Structured block - An executable statement with a single entry at the
+  // top and a single exit at the bottom.
+  // The point of exit cannot be a branch out of the structured block.
+  // longjmp() and throw() must not violate the entry/exit criteria.
+  cast(AStmt)->getCapturedDecl()->setNothrow();
+
   setFunctionHasBranchProtectedScope();
 
   return OMPMasterDirective::Create(Context, StartLoc, EndLoc, AStmt);


Index: test/AST/ast-dump-openmp-master.cpp
===
--- /dev/null
+++ test/AST/ast-dump-openmp-master.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -verify -fopenmp -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp-simd -ast-dump %s | FileCheck %s
+// expected-no-diagnostics
+
+void master() {
+#pragma omp master
+  {
+  }
+}
+
+// CHECK: `-FunctionDecl
+// CHECK-NEXT:   `-CompoundStmt
+// CHECK-NEXT: `-OMPMasterDirective
+// CHECK-NEXT:   `-CapturedStmt
+// CHECK-NEXT: `-CapturedDecl {{.*}} nothrow
+// CHECK-NEXT:   |-CompoundStmt
+// CHECK-NEXT:   `-ImplicitParamDecl
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -6050,6 +6050,13 @@
 
   assert(isa(AStmt) && "Captured statement expected");
 
+  // 1.2.2 OpenMP Language Terminology
+  // Structured block - An executable statement with a single entry at the
+  // top and a single exit at the bottom.
+  // The point of exit cannot be a branch out of the structured block.
+  // longjmp() and throw() must not violate the entry/exit criteria.
+  cast(AStmt)->getCapturedDecl()->setNothrow();
+
   setFunctionHasBranchProtectedScope();
 
   return OMPMasterDirective::Create(Context, StartLoc, EndLoc, AStmt);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits