[OpenACC]Reimplement 'for' loop checks for a loop construct The 'loop' construct has some pretty strict checks as to what the for loop associated with it has to look like. The previous implementation was a little convoluted and missed implementing the 'condition' checking. Additionally, it did a bad job double-diagnosing with templates. This patch rewrites it in a way that does a much better job with the double-diagnosing, and proeprly implements some checks for the 'condition' of the for loop.
diff --git a/clang/include/clang/Sema/SemaOpenACC.h b/clang/include/clang/Sema/SemaOpenACC.h index 2c9134b..47947d9 100644 --- a/clang/include/clang/Sema/SemaOpenACC.h +++ b/clang/include/clang/Sema/SemaOpenACC.h
@@ -125,38 +125,73 @@ /// 'loop' clause enforcement, where this is 'blocked' by a compute construct. llvm::SmallVector<OpenACCReductionClause *> ActiveReductionClauses; - // Type to check the info about the 'for stmt'. - struct ForStmtBeginChecker { + // Type to check the 'for' (or range-for) statement for compatibility with the + // 'loop' directive. + class ForStmtBeginChecker { SemaOpenACC &SemaRef; SourceLocation ForLoc; - bool IsRangeFor = false; - std::optional<const CXXForRangeStmt *> RangeFor = nullptr; - const Stmt *Init = nullptr; - bool InitChanged = false; - std::optional<const Stmt *> Cond = nullptr; - std::optional<const Stmt *> Inc = nullptr; + bool IsInstantiation = false; + + struct RangeForInfo { + const CXXForRangeStmt *Uninstantiated = nullptr; + const CXXForRangeStmt *CurrentVersion = nullptr; + }; + + struct ForInfo { + const Stmt *Init = nullptr; + const Stmt *Condition = nullptr; + const Stmt *Increment = nullptr; + }; + + struct CheckForInfo { + ForInfo Uninst; + ForInfo Current; + }; + + std::variant<RangeForInfo, CheckForInfo> Info; // Prevent us from checking 2x, which can happen with collapse & tile. bool AlreadyChecked = false; - ForStmtBeginChecker(SemaOpenACC &SemaRef, SourceLocation ForLoc, - std::optional<const CXXForRangeStmt *> S) - : SemaRef(SemaRef), ForLoc(ForLoc), IsRangeFor(true), RangeFor(S) {} + void checkRangeFor(); + bool checkForInit(const Stmt *InitStmt, const ValueDecl *&InitVar, + bool Diag); + bool checkForCond(const Stmt *CondStmt, const ValueDecl *InitVar, + bool Diag); + bool checkForInc(const Stmt *IncStmt, const ValueDecl *InitVar, bool Diag); + + void checkFor(); + + // void checkRangeFor(); ?? ERICH + // const ValueDecl *checkInit(); + // void checkCond(const ValueDecl *Init); + // void checkInc(const ValueDecl *Init); + public: + // Checking for non-instantiation version of a Range-for. ForStmtBeginChecker(SemaOpenACC &SemaRef, SourceLocation ForLoc, - const Stmt *I, bool InitChanged, - std::optional<const Stmt *> C, - std::optional<const Stmt *> Inc) - : SemaRef(SemaRef), ForLoc(ForLoc), IsRangeFor(false), Init(I), - InitChanged(InitChanged), Cond(C), Inc(Inc) {} - // Do the checking for the For/Range-For. Currently this implements the 'not - // seq' restrictions only, and should be called either if we know we are a - // top-level 'for' (the one associated via associated-stmt), or extended via - // 'collapse'. + const CXXForRangeStmt *RangeFor) + : SemaRef(SemaRef), ForLoc(ForLoc), IsInstantiation(false), + Info(RangeForInfo{nullptr, RangeFor}) {} + // Checking for an instantiation of the range-for. + ForStmtBeginChecker(SemaOpenACC &SemaRef, SourceLocation ForLoc, + const CXXForRangeStmt *OldRangeFor, + const CXXForRangeStmt *RangeFor) + : SemaRef(SemaRef), ForLoc(ForLoc), IsInstantiation(true), + Info(RangeForInfo{OldRangeFor, RangeFor}) {} + // Checking for a non-instantiation version of a traditional for. + ForStmtBeginChecker(SemaOpenACC &SemaRef, SourceLocation ForLoc, + const Stmt *Init, const Stmt *Cond, const Stmt *Inc) + : SemaRef(SemaRef), ForLoc(ForLoc), IsInstantiation(false), + Info(CheckForInfo{{}, {Init, Cond, Inc}}) {} + // Checking for an instantiation version of a traditional for. + ForStmtBeginChecker(SemaOpenACC &SemaRef, SourceLocation ForLoc, + const Stmt *OldInit, const Stmt *OldCond, + const Stmt *OldInc, const Stmt *Init, const Stmt *Cond, + const Stmt *Inc) + : SemaRef(SemaRef), ForLoc(ForLoc), IsInstantiation(true), + Info(CheckForInfo{{OldInit, OldCond, OldInc}, {Init, Cond, Inc}}) {} + void check(); - - const ValueDecl *checkInit(); - void checkCond(); - void checkInc(const ValueDecl *Init); }; /// Helper function for checking the 'for' and 'range for' stmts.
diff --git a/clang/lib/Sema/SemaOpenACC.cpp b/clang/lib/Sema/SemaOpenACC.cpp index 74eee9b..fc191bd 100644 --- a/clang/lib/Sema/SemaOpenACC.cpp +++ b/clang/lib/Sema/SemaOpenACC.cpp
@@ -944,6 +944,7 @@ LoopInfo.TopLevelLoopSeen = true; if (CollapseInfo.CurCollapseCount && *CollapseInfo.CurCollapseCount > 0) { + // Check the format of this loop if it is affected by the collapse. C.check(); // OpenACC 3.3 2.9.1: @@ -969,6 +970,7 @@ } if (TileInfo.CurTileCount && *TileInfo.CurTileCount > 0) { + // Check the format of this loop if it is affected by the tile. C.check(); if (LoopInfo.CurLevelHasLoopAlready) { @@ -1044,13 +1046,11 @@ if (IsRandomAccessIteratorTag(ItrCategoryDecl)) return true; - // We can also support types inherited from the + // We can also support tag-types inherited from the // random_access_iterator_tag. - for (CXXBaseSpecifier BS : ItrCategoryDecl->bases()) { - + for (CXXBaseSpecifier BS : ItrCategoryDecl->bases()) if (IsRandomAccessIteratorTag(BS.getType()->getAsCXXRecordDecl())) return true; - } return false; } @@ -1058,13 +1058,404 @@ return false; } +const ValueDecl *getDeclFromExpr(const Expr *E) { + E = E->IgnoreParenImpCasts(); + if (const auto *FE = dyn_cast<FullExpr>(E)) + E = FE->getSubExpr(); + E = E->IgnoreParenImpCasts(); + + if (!E) + return nullptr; + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + return dyn_cast<ValueDecl>(DRE->getDecl()); + + if (const auto *ME = dyn_cast<MemberExpr>(E)) + if (isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts())) + return ME->getMemberDecl(); + + return nullptr; +} } // namespace +void SemaOpenACC::ForStmtBeginChecker::checkRangeFor() { + const RangeForInfo &RFI = std::get<RangeForInfo>(Info); + // If this hasn't changed since last instantiated we're done. + if (RFI.Uninstantiated == RFI.CurrentVersion) + return; + + const DeclStmt *UninstRangeStmt = + IsInstantiation ? RFI.Uninstantiated->getBeginStmt() : nullptr; + const DeclStmt *RangeStmt = RFI.CurrentVersion->getBeginStmt(); + + // If this isn't the first time we've checked this loop, suppress any cases + // where we previously diagnosed. + if (UninstRangeStmt) { + const ValueDecl *InitVar = + cast<ValueDecl>(UninstRangeStmt->getSingleDecl()); + QualType VarType = InitVar->getType().getNonReferenceType(); + + if (!isValidLoopVariableType(VarType)) + return; + } + + // In some dependent contexts, the autogenerated range statement doesn't get + // included until instantiation, so skip for now. + if (RangeStmt) { + const ValueDecl *InitVar = cast<ValueDecl>(RangeStmt->getSingleDecl()); + QualType VarType = InitVar->getType().getNonReferenceType(); + + if (!isValidLoopVariableType(VarType)) { + SemaRef.Diag(InitVar->getBeginLoc(), diag::err_acc_loop_variable_type) + << SemaRef.LoopWithoutSeqInfo.Kind << VarType; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + return; + } + } +} +bool SemaOpenACC::ForStmtBeginChecker::checkForInit(const Stmt *InitStmt, + const ValueDecl *&InitVar, + bool Diag) { + // Init statement is required. + if (!InitStmt) { + if (Diag) { + SemaRef.Diag(ForLoc, diag::err_acc_loop_variable) + << SemaRef.LoopWithoutSeqInfo.Kind; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + return true; + } + auto DiagLoopVar = [this, Diag, InitStmt]() { + if (Diag) { + SemaRef.Diag(InitStmt->getBeginLoc(), diag::err_acc_loop_variable) + << SemaRef.LoopWithoutSeqInfo.Kind; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + return true; + }; + + if (const auto *ExprTemp = dyn_cast<ExprWithCleanups>(InitStmt)) + InitStmt = ExprTemp->getSubExpr(); + if (const auto *E = dyn_cast<Expr>(InitStmt)) + InitStmt = E->IgnoreParenImpCasts(); + + InitVar = nullptr; + if (const auto *BO = dyn_cast<BinaryOperator>(InitStmt)) { + // Allow assignment operator here. + + if (!BO->isAssignmentOp()) + return DiagLoopVar(); + + const Expr *LHS = BO->getLHS()->IgnoreParenImpCasts(); + if (const auto *DRE = dyn_cast<DeclRefExpr>(LHS)) + InitVar = DRE->getDecl(); + } else if (const auto *DS = dyn_cast<DeclStmt>(InitStmt)) { + // Allow T t = <whatever> + if (!DS->isSingleDecl()) + return DiagLoopVar(); + InitVar = dyn_cast<ValueDecl>(DS->getSingleDecl()); + + // Ensure we have an initializer, unless this is a record/dependent type. + if (InitVar) { + if (!isa<VarDecl>(InitVar)) + return DiagLoopVar(); + + if (!InitVar->getType()->isRecordType() && + !InitVar->getType()->isDependentType() && + !cast<VarDecl>(InitVar)->hasInit()) + return DiagLoopVar(); + } + } else if (auto *CE = dyn_cast<CXXOperatorCallExpr>(InitStmt)) { + // Allow assignment operator call. + if (CE->getOperator() != OO_Equal) + return DiagLoopVar(); + + const Expr *LHS = CE->getArg(0)->IgnoreParenImpCasts(); + if (auto *DRE = dyn_cast<DeclRefExpr>(LHS)) { + InitVar = DRE->getDecl(); + } else if (auto *ME = dyn_cast<MemberExpr>(LHS)) { + if (isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts())) + InitVar = ME->getMemberDecl(); + } + } + + // If after all of that, we haven't found a variable, give up. + if (!InitVar) + return DiagLoopVar(); + + InitVar = cast<ValueDecl>(InitVar->getCanonicalDecl()); + QualType VarType = InitVar->getType().getNonReferenceType(); + + // Since we have one, all we need to do is ensure it is the right type. + if (!isValidLoopVariableType(VarType)) { + if (Diag) { + SemaRef.Diag(InitVar->getBeginLoc(), diag::err_acc_loop_variable_type) + << SemaRef.LoopWithoutSeqInfo.Kind << VarType; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + return true; + } + + return false; +} + +bool SemaOpenACC::ForStmtBeginChecker::checkForCond(const Stmt *CondStmt, + const ValueDecl *InitVar, + bool Diag) { + // A condition statement is required. + if (!CondStmt) { + if (Diag) { + SemaRef.Diag(ForLoc, diag::err_acc_loop_terminating_condition) + << SemaRef.LoopWithoutSeqInfo.Kind; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + + return true; + } + auto DiagCondVar = [this, Diag, CondStmt] { + if (Diag) { + SemaRef.Diag(CondStmt->getBeginLoc(), + diag::err_acc_loop_terminating_condition) + << SemaRef.LoopWithoutSeqInfo.Kind; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + return true; + }; + + if (const auto *ExprTemp = dyn_cast<ExprWithCleanups>(CondStmt)) + CondStmt = ExprTemp->getSubExpr(); + if (const auto *E = dyn_cast<Expr>(CondStmt)) + CondStmt = E->IgnoreParenImpCasts(); + + const ValueDecl *CondVar = nullptr; + if (const auto *BO = dyn_cast<BinaryOperator>(CondStmt)) { + switch (BO->getOpcode()) { + default: + return DiagCondVar(); + case BO_EQ: + case BO_LT: + case BO_GT: + case BO_NE: + case BO_LE: + case BO_GE: + break; + } + + // Assign the condition-var to the LHS. If it either comes back null, or + // the LHS doesn't match the InitVar, assign it to the RHS so that 5 < N is + // allowed. + CondVar = getDeclFromExpr(BO->getLHS()); + if (!CondVar || + (InitVar && CondVar->getCanonicalDecl() != InitVar->getCanonicalDecl())) + CondVar = getDeclFromExpr(BO->getRHS()); + + } else if (const auto *CE = dyn_cast<CXXOperatorCallExpr>(CondStmt)) { + // Any of the comparison ops should be ok here, but we don't know how to + // handle spaceship, so disallow for now. + if (!CE->isComparisonOp() || CE->getOperator() == OO_Spaceship) + return DiagCondVar(); + + // Same logic here: Assign it to the LHS, unless the LHS comes back null or + // not equal to the init var. + CondVar = getDeclFromExpr(CE->getArg(0)); + if (!CondVar || + (InitVar && + CondVar->getCanonicalDecl() != InitVar->getCanonicalDecl() && + CE->getNumArgs() > 1)) + CondVar = getDeclFromExpr(CE->getArg(1)); + } else if (const auto *ME = dyn_cast<CXXMemberCallExpr>(CondStmt)) { + // Here there isn't much we can do besides hope it is the right variable. + // Codegen might have to just give up on figuring out trip count in this + // case? + CondVar = getDeclFromExpr(ME->getImplicitObjectArgument()); + } + + if (!CondVar) + return DiagCondVar(); + + // Don't consider this an error unless the init variable was properly set, + // else check to make sure they are the same variable. + if (InitVar && CondVar->getCanonicalDecl() != InitVar->getCanonicalDecl()) + return DiagCondVar(); + + return false; +} + +bool SemaOpenACC::ForStmtBeginChecker::checkForInc(const Stmt *IncStmt, + const ValueDecl *InitVar, + bool Diag) { + if (!IncStmt) { + if (Diag) { + SemaRef.Diag(ForLoc, diag::err_acc_loop_not_monotonic) + << SemaRef.LoopWithoutSeqInfo.Kind; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + return true; + } + auto DiagIncVar = [this, Diag, IncStmt] { + if (Diag) { + SemaRef.Diag(IncStmt->getBeginLoc(), diag::err_acc_loop_not_monotonic) + << SemaRef.LoopWithoutSeqInfo.Kind; + SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, + diag::note_acc_construct_here) + << SemaRef.LoopWithoutSeqInfo.Kind; + } + return true; + }; + + if (const auto *ExprTemp = dyn_cast<ExprWithCleanups>(IncStmt)) + IncStmt = ExprTemp->getSubExpr(); + if (const auto *E = dyn_cast<Expr>(IncStmt)) + IncStmt = E->IgnoreParenImpCasts(); + + const ValueDecl *IncVar = nullptr; + // Here we enforce the monotonically increase/decrease: + if (const auto *UO = dyn_cast<UnaryOperator>(IncStmt)) { + // Allow increment/decrement ops. + if (!UO->isIncrementDecrementOp()) + return DiagIncVar(); + IncVar = getDeclFromExpr(UO->getSubExpr()); + } else if (const auto *BO = dyn_cast<BinaryOperator>(IncStmt)) { + switch (BO->getOpcode()) { + default: + return DiagIncVar(); + case BO_AddAssign: + case BO_SubAssign: + case BO_MulAssign: + case BO_DivAssign: + case BO_Assign: + // += -= *= /= should all be fine here, this should be all of the + // 'monotonical' compound-assign ops. + // Assignment we just give up on, we could do better, and ensure that it + // is a binary/operator expr doing more work, but that seems like a lot + // of work for an error prone check. + break; + } + IncVar = getDeclFromExpr(BO->getLHS()); + } else if (const auto *CE = dyn_cast<CXXOperatorCallExpr>(IncStmt)) { + switch (CE->getOperator()) { + default: + return DiagIncVar(); + case OO_PlusPlus: + case OO_MinusMinus: + case OO_PlusEqual: + case OO_MinusEqual: + case OO_StarEqual: + case OO_SlashEqual: + case OO_Equal: + // += -= *= /= should all be fine here, this should be all of the + // 'monotonical' compound-assign ops. + // Assignment we just give up on, we could do better, and ensure that it + // is a binary/operator expr doing more work, but that seems like a + // lot of work for an error prone check. + break; + } + + IncVar = getDeclFromExpr(CE->getArg(0)); + + } else if (const auto *ME = dyn_cast<CXXMemberCallExpr>(IncStmt)) { + IncVar = getDeclFromExpr(ME->getImplicitObjectArgument()); + // We can't really do much for member expressions, other than hope they are + // doing the right thing, so give up here. + } + + if (!IncVar) + return DiagIncVar(); + + // InitVar shouldn't be null unless there was an error, so don't diagnose if + // that is the case. Else we should ensure that it refers to the loop + // value. + if (InitVar && IncVar->getCanonicalDecl() != InitVar->getCanonicalDecl()) + return DiagIncVar(); + + return false; +} + +void SemaOpenACC::ForStmtBeginChecker::checkFor() { + const CheckForInfo &CFI = std::get<CheckForInfo>(Info); + + if (!IsInstantiation) { + // If this isn't an instantiation, we can just check all of these and + // diagnose. + const ValueDecl *CurInitVar = nullptr; + checkForInit(CFI.Current.Init, CurInitVar, /*Diag=*/true); + checkForCond(CFI.Current.Condition, CurInitVar, /*Diag=*/true); + checkForInc(CFI.Current.Increment, CurInitVar, /*DIag=*/true); + } else { + const ValueDecl *UninstInitVar = nullptr; + // Checking the 'init' section first. We have to always run both versions, + // at minimum with the 'diag' off, so that we can ensure we get the correct + // instantiation var for checking by later ones. + bool UninstInitFailed = + checkForInit(CFI.Uninst.Init, UninstInitVar, /*Diag=*/false); + + // VarDecls are always rebuild because they are dependent, so we can do a + // little work to suppress some of the double checking based on whether the + // type is instantiation dependent. This is imperfect, but will get us most + // cases suppressed. Currently this only handles the 'T t =' case. + auto InitChanged = [=]() { + if (CFI.Uninst.Init == CFI.Current.Init) + return false; + + QualType OldVDTy; + QualType NewVDTy; + + if (const auto *DS = dyn_cast<DeclStmt>(CFI.Uninst.Init)) + if (const VarDecl *VD = dyn_cast_if_present<VarDecl>( + DS->isSingleDecl() ? DS->getSingleDecl() : nullptr)) + OldVDTy = VD->getType(); + if (const auto *DS = dyn_cast<DeclStmt>(CFI.Current.Init)) + if (const VarDecl *VD = dyn_cast_if_present<VarDecl>( + DS->isSingleDecl() ? DS->getSingleDecl() : nullptr)) + NewVDTy = VD->getType(); + + if (OldVDTy.isNull() || NewVDTy.isNull()) + return true; + + return OldVDTy->isInstantiationDependentType() != + NewVDTy->isInstantiationDependentType(); + }; + + // Only diagnose the new 'init' if the previous version didn't fail, AND the + // current init changed meaningfully. + bool ShouldDiagNewInit = !UninstInitFailed && InitChanged(); + const ValueDecl *CurInitVar = nullptr; + checkForInit(CFI.Current.Init, CurInitVar, /*Diag=*/ShouldDiagNewInit); + + // Check the condition and increment only if the previous version passed, + // and this changed. + if (CFI.Uninst.Condition != CFI.Current.Condition && + !checkForCond(CFI.Uninst.Condition, UninstInitVar, /*Diag=*/false)) + checkForCond(CFI.Current.Condition, CurInitVar, /*Diag=*/true); + if (CFI.Uninst.Increment != CFI.Current.Increment && + !checkForInc(CFI.Uninst.Increment, UninstInitVar, /*Diag=*/false)) + checkForInc(CFI.Current.Increment, CurInitVar, /*Diag=*/true); + } +} + void SemaOpenACC::ForStmtBeginChecker::check() { + // If this isn't an active loop without a seq, immediately return, nothing to + // check. if (SemaRef.LoopWithoutSeqInfo.Kind == OpenACCDirectiveKind::Invalid) return; + // If we've already checked, because this is a 'top level' one (and asking + // again because 'tile' and 'collapse' might apply), just return, nothing to + // do here. if (AlreadyChecked) return; AlreadyChecked = true; @@ -1084,251 +1475,10 @@ // pointer, that the compiler generates to iterate the range. it is not the // variable declared by the for loop. - if (IsRangeFor) { - // If the range-for is being instantiated and didn't change, don't - // re-diagnose. - if (!RangeFor.has_value()) - return; - // For a range-for, we can assume everything is 'corect' other than the type - // of the iterator, so check that. - const DeclStmt *RangeStmt = (*RangeFor)->getBeginStmt(); + if (std::holds_alternative<RangeForInfo>(Info)) + return checkRangeFor(); - // In some dependent contexts, the autogenerated range statement doesn't get - // included until instantiation, so skip for now. - if (!RangeStmt) - return; - - const ValueDecl *InitVar = cast<ValueDecl>(RangeStmt->getSingleDecl()); - QualType VarType = InitVar->getType().getNonReferenceType(); - if (!isValidLoopVariableType(VarType)) { - SemaRef.Diag(InitVar->getBeginLoc(), diag::err_acc_loop_variable_type) - << SemaRef.LoopWithoutSeqInfo.Kind << VarType; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, - diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - } - return; - } - - // Else we are in normal 'ForStmt', so we can diagnose everything. - // We only have to check cond/inc if they have changed, but 'init' needs to - // just suppress its diagnostics if it hasn't changed. - const ValueDecl *InitVar = checkInit(); - if (Cond.has_value()) - checkCond(); - if (Inc.has_value()) - checkInc(InitVar); -} -const ValueDecl *SemaOpenACC::ForStmtBeginChecker::checkInit() { - if (!Init) { - if (InitChanged) { - SemaRef.Diag(ForLoc, diag::err_acc_loop_variable) - << SemaRef.LoopWithoutSeqInfo.Kind; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, - diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - } - return nullptr; - } - - auto DiagLoopVar = [&]() { - if (InitChanged) { - SemaRef.Diag(Init->getBeginLoc(), diag::err_acc_loop_variable) - << SemaRef.LoopWithoutSeqInfo.Kind; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, - diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - } - return nullptr; - }; - - if (const auto *ExprTemp = dyn_cast<ExprWithCleanups>(Init)) - Init = ExprTemp->getSubExpr(); - if (const auto *E = dyn_cast<Expr>(Init)) - Init = E->IgnoreParenImpCasts(); - - const ValueDecl *InitVar = nullptr; - - if (const auto *BO = dyn_cast<BinaryOperator>(Init)) { - // Allow assignment operator here. - - if (!BO->isAssignmentOp()) - return DiagLoopVar(); - - const Expr *LHS = BO->getLHS()->IgnoreParenImpCasts(); - - if (const auto *DRE = dyn_cast<DeclRefExpr>(LHS)) - InitVar = DRE->getDecl(); - } else if (const auto *DS = dyn_cast<DeclStmt>(Init)) { - // Allow T t = <whatever> - if (!DS->isSingleDecl()) - return DiagLoopVar(); - - InitVar = dyn_cast<ValueDecl>(DS->getSingleDecl()); - - // Ensure we have an initializer, unless this is a record/dependent type. - - if (InitVar) { - if (!isa<VarDecl>(InitVar)) - return DiagLoopVar(); - - if (!InitVar->getType()->isRecordType() && - !InitVar->getType()->isDependentType() && - !cast<VarDecl>(InitVar)->hasInit()) - return DiagLoopVar(); - } - } else if (auto *CE = dyn_cast<CXXOperatorCallExpr>(Init)) { - // Allow assignment operator call. - if (CE->getOperator() != OO_Equal) - return DiagLoopVar(); - - const Expr *LHS = CE->getArg(0)->IgnoreParenImpCasts(); - - if (auto *DRE = dyn_cast<DeclRefExpr>(LHS)) { - InitVar = DRE->getDecl(); - } else if (auto *ME = dyn_cast<MemberExpr>(LHS)) { - if (isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts())) - InitVar = ME->getMemberDecl(); - } - } - - if (!InitVar) - return DiagLoopVar(); - - InitVar = cast<ValueDecl>(InitVar->getCanonicalDecl()); - QualType VarType = InitVar->getType().getNonReferenceType(); - - // Since we have one, all we need to do is ensure it is the right type. - if (!isValidLoopVariableType(VarType)) { - if (InitChanged) { - SemaRef.Diag(InitVar->getBeginLoc(), diag::err_acc_loop_variable_type) - << SemaRef.LoopWithoutSeqInfo.Kind << VarType; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, - diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - } - return nullptr; - } - - return InitVar; -} -void SemaOpenACC::ForStmtBeginChecker::checkCond() { - if (!*Cond) { - SemaRef.Diag(ForLoc, diag::err_acc_loop_terminating_condition) - << SemaRef.LoopWithoutSeqInfo.Kind; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - } - // Nothing else to do here. we could probably do some additional work to look - // into the termination condition, but that error-prone. For now, we don't - // implement anything other than 'there is a termination condition', and if - // codegen/MLIR comes up with some necessary restrictions, we can implement - // them here. -} - -void SemaOpenACC::ForStmtBeginChecker::checkInc(const ValueDecl *Init) { - - if (!*Inc) { - SemaRef.Diag(ForLoc, diag::err_acc_loop_not_monotonic) - << SemaRef.LoopWithoutSeqInfo.Kind; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - return; - } - auto DiagIncVar = [this] { - SemaRef.Diag((*Inc)->getBeginLoc(), diag::err_acc_loop_not_monotonic) - << SemaRef.LoopWithoutSeqInfo.Kind; - SemaRef.Diag(SemaRef.LoopWithoutSeqInfo.Loc, diag::note_acc_construct_here) - << SemaRef.LoopWithoutSeqInfo.Kind; - return; - }; - - if (const auto *ExprTemp = dyn_cast<ExprWithCleanups>(*Inc)) - Inc = ExprTemp->getSubExpr(); - if (const auto *E = dyn_cast<Expr>(*Inc)) - Inc = E->IgnoreParenImpCasts(); - - auto getDeclFromExpr = [](const Expr *E) -> const ValueDecl * { - E = E->IgnoreParenImpCasts(); - if (const auto *FE = dyn_cast<FullExpr>(E)) - E = FE->getSubExpr(); - - E = E->IgnoreParenImpCasts(); - - if (!E) - return nullptr; - if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) - return dyn_cast<ValueDecl>(DRE->getDecl()); - - if (const auto *ME = dyn_cast<MemberExpr>(E)) - if (isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts())) - return ME->getMemberDecl(); - - return nullptr; - }; - - const ValueDecl *IncVar = nullptr; - - // Here we enforce the monotonically increase/decrease: - if (const auto *UO = dyn_cast<UnaryOperator>(*Inc)) { - // Allow increment/decrement ops. - if (!UO->isIncrementDecrementOp()) - return DiagIncVar(); - IncVar = getDeclFromExpr(UO->getSubExpr()); - } else if (const auto *BO = dyn_cast<BinaryOperator>(*Inc)) { - switch (BO->getOpcode()) { - default: - return DiagIncVar(); - case BO_AddAssign: - case BO_SubAssign: - case BO_MulAssign: - case BO_DivAssign: - case BO_Assign: - // += -= *= /= should all be fine here, this should be all of the - // 'monotonical' compound-assign ops. - // Assignment we just give up on, we could do better, and ensure that it - // is a binary/operator expr doing more work, but that seems like a lot - // of work for an error prone check. - break; - } - IncVar = getDeclFromExpr(BO->getLHS()); - } else if (const auto *CE = dyn_cast<CXXOperatorCallExpr>(*Inc)) { - switch (CE->getOperator()) { - default: - return DiagIncVar(); - case OO_PlusPlus: - case OO_MinusMinus: - case OO_PlusEqual: - case OO_MinusEqual: - case OO_StarEqual: - case OO_SlashEqual: - case OO_Equal: - // += -= *= /= should all be fine here, this should be all of the - // 'monotonical' compound-assign ops. - // Assignment we just give up on, we could do better, and ensure that it - // is a binary/operator expr doing more work, but that seems like a lot - // of work for an error prone check. - break; - } - - IncVar = getDeclFromExpr(CE->getArg(0)); - - } else if (const auto *ME = dyn_cast<CXXMemberCallExpr>(*Inc)) { - IncVar = getDeclFromExpr(ME->getImplicitObjectArgument()); - // We can't really do much for member expressions, other than hope they are - // doing the right thing, so give up here. - } - - if (!IncVar) - return DiagIncVar(); - - // InitVar shouldn't be null unless there was an error, so don't diagnose if - // that is the case. Else we should ensure that it refers to the loop - // value. - if (Init && IncVar->getCanonicalDecl() != Init->getCanonicalDecl()) - return DiagIncVar(); - - return; + return checkFor(); } void SemaOpenACC::ActOnForStmtBegin(SourceLocation ForLoc, const Stmt *OldFirst, @@ -1338,41 +1488,10 @@ if (!getLangOpts().OpenACC) return; - std::optional<const Stmt *> S; - if (OldSecond == Second) - S = std::nullopt; - else - S = Second; - std::optional<const Stmt *> T; - if (OldThird == Third) - S = std::nullopt; - else - S = Third; - - bool InitChanged = false; - if (OldFirst != First) { - InitChanged = true; - - // VarDecls are always rebuild because they are dependent, so we can do a - // little work to suppress some of the double checking based on whether the - // type is instantiation dependent. - QualType OldVDTy; - QualType NewVDTy; - if (const auto *DS = dyn_cast<DeclStmt>(OldFirst)) - if (const VarDecl *VD = dyn_cast_if_present<VarDecl>( - DS->isSingleDecl() ? DS->getSingleDecl() : nullptr)) - OldVDTy = VD->getType(); - if (const auto *DS = dyn_cast<DeclStmt>(First)) - if (const VarDecl *VD = dyn_cast_if_present<VarDecl>( - DS->isSingleDecl() ? DS->getSingleDecl() : nullptr)) - NewVDTy = VD->getType(); - - if (!OldVDTy.isNull() && !NewVDTy.isNull()) - InitChanged = OldVDTy->isInstantiationDependentType() != - NewVDTy->isInstantiationDependentType(); - } - - ForStmtBeginChecker FSBC{*this, ForLoc, First, InitChanged, S, T}; + ForStmtBeginChecker FSBC{*this, ForLoc, OldFirst, OldSecond, + OldThird, First, Second, Third}; + // Check if this is the top-level 'for' for a 'loop'. Else it will be checked + // as a part of the helper if a tile/collapse applies. if (!LoopInfo.TopLevelLoopSeen) { FSBC.check(); } @@ -1385,11 +1504,12 @@ if (!getLangOpts().OpenACC) return; - ForStmtBeginChecker FSBC{*this, ForLoc, First, /*InitChanged=*/true, - Second, Third}; - if (!LoopInfo.TopLevelLoopSeen) { + ForStmtBeginChecker FSBC{*this, ForLoc, First, Second, Third}; + + // Check if this is the top-level 'for' for a 'loop'. Else it will be checked + // as a part of the helper if a tile/collapse applies. + if (!LoopInfo.TopLevelLoopSeen) FSBC.check(); - } ForStmtBeginHelper(ForLoc, FSBC); } @@ -1400,14 +1520,11 @@ if (!getLangOpts().OpenACC) return; - std::optional<const CXXForRangeStmt *> RF; - - if (OldRangeFor == RangeFor) - RF = std::nullopt; - else - RF = cast<CXXForRangeStmt>(RangeFor); - - ForStmtBeginChecker FSBC{*this, ForLoc, RF}; + ForStmtBeginChecker FSBC{*this, ForLoc, + cast_if_present<CXXForRangeStmt>(OldRangeFor), + cast_if_present<CXXForRangeStmt>(RangeFor)}; + // Check if this is the top-level 'for' for a 'loop'. Else it will be checked + // as a part of the helper if a tile/collapse applies. if (!LoopInfo.TopLevelLoopSeen) { FSBC.check(); } @@ -1419,10 +1536,14 @@ if (!getLangOpts().OpenACC) return; - ForStmtBeginChecker FSBC{*this, ForLoc, cast<CXXForRangeStmt>(RangeFor)}; - if (!LoopInfo.TopLevelLoopSeen) { + ForStmtBeginChecker FSBC = {*this, ForLoc, + cast_if_present<CXXForRangeStmt>(RangeFor)}; + + // Check if this is the top-level 'for' for a 'loop'. Else it will be checked + // as a part of the helper if a tile/collapse applies. + if (!LoopInfo.TopLevelLoopSeen) FSBC.check(); - } + ForStmtBeginHelper(ForLoc, FSBC); }
diff --git a/clang/test/SemaOpenACC/combined-construct.cpp b/clang/test/SemaOpenACC/combined-construct.cpp index b0fd05e..e7ca0ee 100644 --- a/clang/test/SemaOpenACC/combined-construct.cpp +++ b/clang/test/SemaOpenACC/combined-construct.cpp
@@ -176,8 +176,8 @@ for(int f;;); #pragma acc kernels loop - // expected-error@+6 2{{OpenACC 'kernels loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'kernels loop' construct is here}} + // expected-error@+6{{OpenACC 'kernels loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'kernels loop' construct is here}} // expected-error@+4{{OpenACC 'kernels loop' construct must have a terminating condition}} // expected-note@-4{{'kernels loop' construct is here}} // expected-error@+2{{OpenACC 'kernels loop' variable must monotonically increase or decrease ('++', '--', or compound assignment)}} @@ -255,8 +255,8 @@ for( i = 0;;); #pragma acc kernels loop - // expected-error@+6 2{{OpenACC 'kernels loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'kernels loop' construct is here}} + // expected-error@+6{{OpenACC 'kernels loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'kernels loop' construct is here}} // expected-error@+4{{OpenACC 'kernels loop' construct must have a terminating condition}} // expected-note@-4{{'kernels loop' construct is here}} // expected-error@+2{{OpenACC 'kernels loop' variable must monotonically increase or decrease ('++', '--', or compound assignment)}} @@ -273,8 +273,8 @@ for( int j ;;); #pragma acc serial loop - // expected-error@+6 2{{OpenACC 'serial loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'serial loop' construct is here}} + // expected-error@+6{{OpenACC 'serial loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'serial loop' construct is here}} // expected-error@+4{{OpenACC 'serial loop' construct must have a terminating condition}} // expected-note@-4{{'serial loop' construct is here}} // expected-error@+2{{OpenACC 'serial loop' variable must monotonically increase or decrease ('++', '--', or compound assignment)}} @@ -342,8 +342,8 @@ for(auto X : Array); #pragma acc kernels loop - // expected-error@+2 2{{loop variable of loop associated with an OpenACC 'kernels loop' construct must be of integer, pointer, or random-access-iterator type (is 'SomeIterator')}} - // expected-note@-2 2{{'kernels loop' construct is here}} + // expected-error@+2{{loop variable of loop associated with an OpenACC 'kernels loop' construct must be of integer, pointer, or random-access-iterator type (is 'SomeIterator')}} + // expected-note@-2{{'kernels loop' construct is here}} for(auto X : HasIteratorCollection{}); #pragma acc serial loop @@ -355,8 +355,8 @@ RandAccessIterator f; #pragma acc serial loop - // expected-error@+2 2{{OpenACC 'serial loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'serial loop' construct is here}} + // expected-error@+2{{OpenACC 'serial loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'serial loop' construct is here}} for(f;f != end;f++); #pragma acc kernels loop
diff --git a/clang/test/SemaOpenACC/loop-construct.cpp b/clang/test/SemaOpenACC/loop-construct.cpp index 5616cac..7509811 100644 --- a/clang/test/SemaOpenACC/loop-construct.cpp +++ b/clang/test/SemaOpenACC/loop-construct.cpp
@@ -167,8 +167,8 @@ for(int f;;); #pragma acc loop - // expected-error@+6 2{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'loop' construct is here}} + // expected-error@+6{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'loop' construct is here}} // expected-error@+4{{OpenACC 'loop' construct must have a terminating condition}} // expected-note@-4{{'loop' construct is here}} // expected-error@+2{{OpenACC 'loop' variable must monotonically increase or decrease ('++', '--', or compound assignment)}} @@ -246,8 +246,8 @@ for( i = 0;;); #pragma acc loop - // expected-error@+6 2{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'loop' construct is here}} + // expected-error@+6{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'loop' construct is here}} // expected-error@+4{{OpenACC 'loop' construct must have a terminating condition}} // expected-note@-4{{'loop' construct is here}} // expected-error@+2{{OpenACC 'loop' variable must monotonically increase or decrease ('++', '--', or compound assignment)}} @@ -264,8 +264,8 @@ for( int j ;;); #pragma acc loop - // expected-error@+6 2{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'loop' construct is here}} + // expected-error@+6{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'loop' construct is here}} // expected-error@+4{{OpenACC 'loop' construct must have a terminating condition}} // expected-note@-4{{'loop' construct is here}} // expected-error@+2{{OpenACC 'loop' variable must monotonically increase or decrease ('++', '--', or compound assignment)}} @@ -333,8 +333,8 @@ for(auto X : Array); #pragma acc loop - // expected-error@+2 2{{loop variable of loop associated with an OpenACC 'loop' construct must be of integer, pointer, or random-access-iterator type (is 'SomeIterator')}} - // expected-note@-2 2{{'loop' construct is here}} + // expected-error@+2{{loop variable of loop associated with an OpenACC 'loop' construct must be of integer, pointer, or random-access-iterator type (is 'SomeIterator')}} + // expected-note@-2{{'loop' construct is here}} for(auto X : HasIteratorCollection{}); #pragma acc loop @@ -346,8 +346,8 @@ RandAccessIterator f; #pragma acc loop - // expected-error@+2 2{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} - // expected-note@-2 2{{'loop' construct is here}} + // expected-error@+2{{OpenACC 'loop' construct must have initialization clause in canonical form ('var = init' or 'T var = init'}} + // expected-note@-2{{'loop' construct is here}} for(f;f != end;f++); #pragma acc loop @@ -373,6 +373,25 @@ #pragma acc loop for(f = 0;f != end;f+=1); + +#pragma acc loop + for (int i = 0; 5 >= i; ++i); + + int otherI; + // expected-error@+3{{OpenACC 'loop' construct must have a terminating condition}} + // expected-note@+1{{'loop' construct is here}} +#pragma acc loop + for (int i = 0; otherI != 5; ++i); + + // expected-error@+3{{OpenACC 'loop' construct must have a terminating condition}} + // expected-note@+1{{'loop' construct is here}} +#pragma acc loop + for (int i = 0; i != 5 && i < 3; ++i); + + // expected-error@+3{{OpenACC 'loop' variable must monotonically increase or decrease}} + // expected-note@+1{{'loop' construct is here}} +#pragma acc loop + for (int i = 0; i != 5; ++f); } void inst() {