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

[Mike Rice via Phabricator via llvm-commits]

mikerice added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:9766-9768
+namespace {
+class DispatchStmtExprChecker
+    : public StmtVisitor<DispatchStmtExprChecker, bool> {
----------------
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.


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

https://reviews.llvm.org/D99537