[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:
> > > > 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();
  }
```


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

https://reviews.llvm.org/D99537