lebedev.ri 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:
> 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.
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
