[PATCH] D99537: [OPENMP51]Initial support for the dispatch directive

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:9766-9768
+namespace {
+class DispatchStmtExprChecker
+    : public StmtVisitor<DispatchStmtExprChecker, bool> {
----------------
mikerice wrote:
> ABataev wrote:
> > mikerice wrote:
> > > ABataev wrote:
> > > > mikerice wrote:
> > > > > ABataev wrote:
> > > > > > Why `Expr->IgnoreParenImpCasts()` does not work here?
> > > > > No reason not to use IgnoreParenImpCasts() instead of IgnoreParens() as far as I know.  That should eliminate the need for visiting MaterializeTemporaryExpr.  It unfortunately doesn't skip over CXXBindTemporaryExpr.  Or were you suggesting something else?
> > > > Then just extend `IgnoreParenImpCasts` to support `MaterializeTemporaryExpr` and control it by the parameter.
> > > CXXBindTemporaryExpr isn't the only reason to use the visitor.  It ends up handling the top level statements and ExprWithCleanups easily and is fairly understandable to me.  I went ahead and hand wrote the code to see what it would look like.  Turns out to be long chains of one line if statement like this:
> > > 
> > > 
> > > ```
> > > static Expr *getDirectCallExpr(Expr *E) {
> > >   E = E->IgnoreParenImpCasts();
> > >   if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
> > >     E = BTE->getSubExpr();
> > >   if (auto *CE = dyn_cast<CallExpr>(E))
> > >     if (CE->getDirectCallee())
> > >       return E;
> > >   return nullptr;
> > > }
> > > 
> > >   SourceLocation TargetCallLoc;
> > > 
> > >   if (!CurContext->isDependentContext()) {
> > >     Expr *TargetCall = nullptr;
> > > 
> > >     auto *E = dyn_cast<Expr>(S);
> > >     if (!E) {
> > >       Diag(S->getBeginLoc(), diag::err_omp_dispatch_statement_call);
> > >       return StmtError();
> > >     }
> > > 
> > >     if (auto *EWC = dyn_cast<ExprWithCleanups>(S))
> > >       E = EWC->getSubExpr();
> > > 
> > >     E = E->IgnoreParenImpCasts();
> > > 
> > >     if (auto *CastE = dyn_cast<CastExpr>(E))
> > >       if (CastE->getCastKind() == CK_ToVoid)
> > >         E = CastE->getSubExpr()->IgnoreParenImpCasts();
> > > 
> > >     if (auto *COCE = dyn_cast<CXXOperatorCallExpr>(E)) {
> > >       if (COCE->getOperator() == OO_Equal) {
> > >         if (Expr *TE = getDirectCallExpr(COCE->getArg(1)))
> > >           TargetCall = TE;
> > >         else if (COCE->getDirectCallee())
> > >           TargetCall = COCE;
> > >       }
> > >     } else if (auto *BO = dyn_cast<BinaryOperator>(E)) {
> > >       if (BO->getOpcode() == BO_Assign)
> > >         if (Expr *TE = getDirectCallExpr(BO->getRHS()))
> > >           TargetCall = TE;
> > >     } else if (auto *CE = dyn_cast<CallExpr>(E)) {
> > >       if (CE->getDirectCallee())
> > >         TargetCall = E;
> > >     }
> > >     if (!TargetCall) {
> > >       Diag(E->getBeginLoc(), diag::err_omp_dispatch_statement_call);
> > >       return StmtError();
> > >     }
> > >     TargetCallLoc = TargetCall->getExprLoc();
> > >   }
> > > 
> > > ```
> > > I didn't originally write the visitor code so I am not attached to it or anything.  Do you think this is better?  Is there some rule-of-thumb when to use visitors and when to write it out by hand?
> > I think something like this will work
> > ```
> > static Expr *getDirectCallExpr(Expr *E) {
> >   E = E->IgnoreParenCasts();
> >   if (auto *CE = dyn_cast<CallExpr>(E))
> >     if (CE->getDirectCallee())
> >       return E;
> >   return nullptr;
> > }
> > 
> >   SourceLocation TargetCallLoc;
> > 
> >   if (!CurContext->isDependentContext()) {
> >     Expr *TargetCall = nullptr;
> > 
> >     auto *E = dyn_cast<Expr>(S);
> >     if (!E) {
> >       Diag(S->getBeginLoc(), diag::err_omp_dispatch_statement_call);
> >       return StmtError();
> >     }
> > 
> >     E = E->IgnoreParenCasts();
> > 
> >     if (auto *BO = dyn_cast<BinaryOperator>(E)) {
> >       if (BO->getOpcode() == BO_Assign)
> >         TargetCall = getDirectCallExpr(BO->getRHS());
> >     } else {
> >       if (auto *COCE = dyn_cast<CXXOperatorCallExpr>(E))
> >         if (COCE->getOperator() == OO_Equal)
> >           TargetCall = getDirectCallExpr(COCE->getArg(1));
> >       if (!TargetCall)
> >         TargetCall = getDirectCallExpr(E);
> >     }
> >     if (!TargetCall) {
> >       Diag(E->getBeginLoc(), diag::err_omp_dispatch_statement_call);
> >       return StmtError();
> >     }
> >     TargetCallLoc = TargetCall->getExprLoc();
> >   }
> > ```
> Thanks. That's pretty good but it does not handle the CXXBindTemporaryExpr so the test:
> 
> ```
> Obj o;
> o = foo_obj();
> 
> CXXOperatorCallExpr 0x5b50b0 'struct Obj' lvalue '='
> |-ImplicitCastExpr 0x5b5098 'struct Obj &(*)(const struct Obj &) noexcept' <FunctionToPointerDecay>
> | `-DeclRefExpr 0x5b4f98 'struct Obj &(const struct Obj &) noexcept' lvalue CXXMethod 0x5b4de8 'operator=' 'struct Obj &(const struct Obj &) noexcept'
> |-DeclRefExpr 0x5b4d00 'struct Obj' lvalue Var 0x5b4830 'o' 'struct Obj'
> `-MaterializeTemporaryExpr 0x5b4f80 'const struct Obj' lvalue
>   `-ImplicitCastExpr 0x5b4f68 'const struct Obj' <NoOp>
>     `-CXXBindTemporaryExpr 0x5b4dc8 'struct Obj' (CXXTemporary 0x5b4dc8)
>       `-CallExpr 0x5b4da0 'struct Obj'
>         `-ImplicitCastExpr 0x5b4d88 'struct Obj (*)(void)' <FunctionToPointerDecay>
>           `-DeclRefExpr 0x5b4d68 'struct Obj (void)' lvalue Function 0x5af938 'foo_obj' 'struct Obj (void)'
> ```
> 
> Would choose the operator= call as the target instead of foo_obj().  I am assuming the user would want foo_obj here.
> 
> I am not sure there is a single 'Ignore' call that works here.  And this case has two levels.  An additional call to IgnoreImplicit() in getDirectCallExpr it should take care of it though.  I'll upload a new patch.
Yes, just add `IgnoreImplicit()` call


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

https://reviews.llvm.org/D99537