[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In D59214#1917149 <https://reviews.llvm.org/D59214#1917149>, @lebedev.ri wrote:

> In D59214#1916596 <https://reviews.llvm.org/D59214#1916596>, @sammccall wrote:
>
> > So if I understand the history here:
> >
> > - the `IsOMPStructuredBlock` bit on `Stmt` was added to implement `getStructuredBlock()`
> > - after review, `getStructuredBlock()` doesn't use the bit
> > - the bit is used in the textual AST dump, and an AST matcher (which is in turn never used in-tree)
> >
> >   Can we have the bit back please :-) We're currently trying to add a HasErrors bit to improve error-recovery and some nodes are completely full. Using a bit on every Stmt to improve the text dump a little and provide a matcher (only relevant to OMP files) seems disproportionately expensive.
>
>
>
>
> In D59214#1917106 <https://reviews.llvm.org/D59214#1917106>, @rjmccall wrote:
>
> > If the bit is unused except for propagation, please remove it.
>
>
> This review (& https://bugs.llvm.org/show_bug.cgi?id=40563 in general)
>  has left me so burnout as to all this openmp stuff that i never
>  followed-up with the actual planned usages of this bit.


I'm sorry to hear that.

> The reasons why this bit is needed (i think they were spelled out here)
>  still apply. One obvious example is https://bugs.llvm.org/show_bug.cgi?id=41102,
>  where after we'd teach `ExceptionAnalyzer` to look into `CapturedStmt` / `CapturedDecl`,
>  we'd immediately have false-positives for these OpenMP structured blocks,
>  so we'd need to avoid traversing into them.
> 
> Doing that via a single bit check should be trivial, as opposed to,
>  i dunno, maintainting a set of all the openmp structured block statements,
>  and before visiting each statement seeing if it is in that set?
>  That kinda sounds slow.

Well, two points.

The first is that language dialects have to take a back-seat to general-purpose
mechanisms.  I care a lot about Objective-C ARC, and that dialect has some
type qualifiers that are used very frequently, but the representation does not
and will never prioritize those type qualifiers over things that are common to
C/C++, and so we just use extra memory when compiling ARC.

The second is that we can pretty easily solve your problem without adding a
bit to `Stmt`.  Captured statements are like blocks in the sense that traverses
that walk into captured statements will always pass through a node in the AST
that knows it's introducing a captured statement, so they should be easy to just
record that the traversal is currently within a captured statement, or even
maintain a stack of such contexts.  If it helps to add "callbacks" to some CRTP
visitor class when entering and exiting a special local context, we can do that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59214