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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional<Stmt *> getStructuredBlockImpl() const {
+    return const_cast<Stmt *>(getInnermostCapturedStmt()->getCapturedStmt());
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > ABataev wrote:
> > > lebedev.ri wrote:
> > > > lebedev.ri wrote:
> > > > > ABataev wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > ABataev wrote:
> > > > > > > > No need to insert it into each class, just add:
> > > > > > > > ```
> > > > > > > > Stmt * OMPExecutableDirective::getStructuredBlock() const {
> > > > > > > >   if (!hasAssociatedStmt() || !getAssociatedStmt())
> > > > > > > >     return nullptr;
> > > > > > > >   if (auto *LD = dyn_cast<OMPLoopDirective>(this))
> > > > > > > >     return LD->getBody();
> > > > > > > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > > > > > > }
> > > > > > > > ```
> > > > > > > I absolutely can do that, you are sure that is the most future-proof state?
> > > > > > > In particular, i want to re-point-out that if it's implemented like this,
> > > > > > > in the base class, then the sub-class may(will) not even know about this function,
> > > > > > > and thus 'forget' to update it, should it not be giving the correct answer for
> > > > > > > that new specific OpenMP executable directive.
> > > > > > > 
> > > > > > > You are sure it's better to implement it in the `OMPExecutableDirective` itself?
> > > > > > Yes, I'm sure. It is the universal solution and all future classes must be compatible with it. If they are not, then they are incorrect.
> > > > > Aha! Well, ok then.
> > > > > 
> > > > > Do you also suggest that `Optional<>` is too fancy?
> > > > > Would it be better to do this instead?
> > > > > ```
> > > > > bool isStandaloneDirective() const {
> > > > >   return !hasAssociatedStmt() || !getAssociatedStmt();
> > > > > }
> > > > > 
> > > > > // Requires: !isStandaloneDirective()
> > > > > Stmt *OMPExecutableDirective::getStructuredBlock() const {
> > > > >   assert(!isStandaloneDirective() && "Standalone Executable OpenMP directives don't have structured blocks.")
> > > > >   if (auto *LD = dyn_cast<OMPLoopDirective>(this))
> > > > >     return LD->getBody();
> > > > >   return getInnermostCapturedStmt()->getCapturedStmt();
> > > > > }
> > > > > ```
> > > > > Hm, maybe that actually conveys more meaning..
> > > > Great, that doesn't work, and highlights my concerns.
> > > > `target enter data` / `target exit data` / `target update` are stand-alone directives as per the spec,
> > > > but not as per that `isStandaloneDirective()` check ^.
> > > > https://godbolt.org/z/0tE93s
> > > > 
> > > > Is this a bug, or intentional?
> > > Well, this is an incompatibility caused by previous not-quite correct implementation. It was reworked already, but these incorrect children still remain, I just had no time to clean them out. You can fix this.
> > Okay, that is reassuring, thanks.
> > this is an incompatibility caused by previous not-quite correct implementation. It was reworked already,
> > but these incorrect children still remain, I just had no time to clean them out. You can fix this.
> 
> I have looked into this, and while parsing/sema changes are trivial, it has consequences for CodeGen.
> In particular `CodeGenFunction::EmitOMPTargetEnterDataDirective()` & friends can no longer
> create `OMPLexicalScope` with `OMPD_task`, or else it will assert that there are no associated stmts.
> 
> And more importantly, in the end `CodeGenFunction::EmitOMPTargetTaskBasedDirective()` tries to, again, 
> get these assoc stmts, and fails. I'm guessing it shouldn't just bailout, then it would not emit anything?
> 
> Any hints would be appreciated.
Ahh, now I recall why it was required. It is needed to support the asynchronous data transfers. It can be reworked to avoid adding those special associated statements but it requires significant amount of time.
You can just add a check for those 3 directives in this `Stmt *OMPExecutableDirective::getStructuredBlock()` and return `nullptr` for them explicitly. I think it is still better than adding a whole bunch of those `getStructuredStmt` functions for each class. Plus, using this single function, you can apply it to the base `OMPExecutableDirective` class rather than to the derived classes. Some matchers could prefer to do this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214