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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
