[OpenACC] Fix uses of getBaseOriginalType when we really want elt type. #162880
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lately, I've been using 'getBaseOriginalType' in ArraySectionExpr incorrectly: it gets the base-ist of element type, when in reality, I want a single type of indirection. This patch corrects the handful of uses that I had for it.
erichkeane
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Erich Keane (erichkeane) ChangesLately, I've been using 'getBaseOriginalType' in ArraySectionExpr incorrectly: it gets the base-ist of element type, when in reality, I want a single type of indirection. This patch corrects the handful of uses that I had for it. Patch is 21.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/162880.diff 10 Files Affected:
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index e1a4005d1a890..573cc72db35c6 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -7160,6 +7160,18 @@ class ArraySectionExpr : public Expr {
/// Return original type of the base expression for array section.
static QualType getBaseOriginalType(const Expr *Base);
+ /// Return the effective 'element' type of this array section. As the array
+ /// section itself returns a collection of elements (closer to its `getBase`
+ /// type), this is only useful for figuring out the effective type of this if
+ /// it were a normal Array subscript expr.
+ QualType getElementType() const;
+
+ /// Returns the effective 'type' of the base of this array section. This
+ /// should be the array/pointer type that this operates on. Just
+ /// getBase->getType isn't sufficient, since it doesn't look through existing
+ /// Array sections to figure out the actual 'base' of this.
+ QualType getBaseType() const;
+
static bool classof(const Stmt *T) {
return T->getStmtClass() == ArraySectionExprClass;
}
diff --git a/clang/include/clang/Sema/SemaOpenACC.h b/clang/include/clang/Sema/SemaOpenACC.h
index 09fdf75fbbd09..6cadc343cd728 100644
--- a/clang/include/clang/Sema/SemaOpenACC.h
+++ b/clang/include/clang/Sema/SemaOpenACC.h
@@ -911,6 +911,7 @@ class SemaOpenACC : public SemaBase {
ExprResult CheckReductionVar(OpenACCDirectiveKind DirectiveKind,
OpenACCReductionOperator ReductionOp,
Expr *VarExpr);
+ bool CheckReductionVarType(Expr *VarExpr);
/// Called to check the 'var' type is a variable of pointer type, necessary
/// for 'deviceptr' and 'attach' clauses. Returns true on success.
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index f899b3c4bb79c..597cbd846e4d9 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -5290,6 +5290,33 @@ QualType ArraySectionExpr::getBaseOriginalType(const Expr *Base) {
return OriginalTy;
}
+QualType ArraySectionExpr::getElementType() const {
+ QualType BaseTy = getBase()->IgnoreParenImpCasts()->getType();
+ // We only have to look into the array section exprs, else we will get the
+ // type of the base, which should already be valid.
+ if (auto *ASE = dyn_cast<ArraySectionExpr>(getBase()->IgnoreParenImpCasts()))
+ BaseTy = ASE->getElementType();
+
+ if (BaseTy->isAnyPointerType())
+ return BaseTy->getPointeeType();
+ if (BaseTy->isArrayType())
+ return BaseTy->castAsArrayTypeUnsafe()->getElementType();
+
+ // If this isn't a pointer or array, the base is a dependent expression, so
+ // just return the BaseTy anyway.
+ assert(BaseTy->isInstantiationDependentType());
+ return BaseTy;
+}
+
+QualType ArraySectionExpr::getBaseType() const {
+ // We only have to look into the array section exprs, else we will get the
+ // type of the base, which should already be valid.
+ if (auto *ASE = dyn_cast<ArraySectionExpr>(getBase()->IgnoreParenImpCasts()))
+ return ASE->getElementType();
+
+ return getBase()->IgnoreParenImpCasts()->getType();
+}
+
RecoveryExpr::RecoveryExpr(ASTContext &Ctx, QualType T, SourceLocation BeginLoc,
SourceLocation EndLoc, ArrayRef<Expr *> SubExprs)
: Expr(RecoveryExprClass, T.getNonReferenceType(),
diff --git a/clang/lib/CIR/CodeGen/CIRGenOpenACC.cpp b/clang/lib/CIR/CodeGen/CIRGenOpenACC.cpp
index 4cf2237468afd..5ba6bcb192b91 100644
--- a/clang/lib/CIR/CodeGen/CIRGenOpenACC.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenOpenACC.cpp
@@ -73,7 +73,7 @@ CIRGenFunction::getOpenACCDataOperandInfo(const Expr *e) {
// Array sections are special, and we have to treat them that way.
if (const auto *section =
dyn_cast<ArraySectionExpr>(curVarExpr->IgnoreParenImpCasts()))
- origType = ArraySectionExpr::getBaseOriginalType(section);
+ origType = section->getElementType();
mlir::Location exprLoc = cgm.getLoc(curVarExpr->getBeginLoc());
llvm::SmallVector<mlir::Value> bounds;
@@ -84,16 +84,10 @@ CIRGenFunction::getOpenACCDataOperandInfo(const Expr *e) {
e->printPretty(os, nullptr, getContext().getPrintingPolicy());
auto addBoundType = [&](const Expr *e) {
- if (const auto *section = dyn_cast<ArraySectionExpr>(curVarExpr)) {
- QualType baseTy = ArraySectionExpr::getBaseOriginalType(
- section->getBase()->IgnoreParenImpCasts());
- if (auto *at = getContext().getAsArrayType(baseTy))
- boundTypes.push_back(at->getElementType());
- else
- boundTypes.push_back(baseTy->getPointeeType());
- } else {
+ if (const auto *section = dyn_cast<ArraySectionExpr>(curVarExpr))
+ boundTypes.push_back(section->getElementType());
+ else
boundTypes.push_back(curVarExpr->getType());
- }
};
addBoundType(curVarExpr);
@@ -113,8 +107,7 @@ CIRGenFunction::getOpenACCDataOperandInfo(const Expr *e) {
if (const Expr *len = section->getLength()) {
extent = emitOpenACCIntExpr(len);
} else {
- QualType baseTy = ArraySectionExpr::getBaseOriginalType(
- section->getBase()->IgnoreParenImpCasts());
+ QualType baseTy = section->getBaseType();
// We know this is the case as implicit lengths are only allowed for
// array types with a constant size, or a dependent size. AND since
// we are codegen we know we're not dependent.
diff --git a/clang/lib/Sema/SemaOpenACC.cpp b/clang/lib/Sema/SemaOpenACC.cpp
index 4824b5a3082a4..f3969a96b8ced 100644
--- a/clang/lib/Sema/SemaOpenACC.cpp
+++ b/clang/lib/Sema/SemaOpenACC.cpp
@@ -2759,7 +2759,7 @@ OpenACCPrivateRecipe SemaOpenACC::CreatePrivateInitRecipe(const Expr *VarExpr) {
// Array sections are special, and we have to treat them that way.
if (const auto *ASE =
dyn_cast<ArraySectionExpr>(VarExpr->IgnoreParenImpCasts()))
- VarTy = ArraySectionExpr::getBaseOriginalType(ASE);
+ VarTy = ASE->getElementType();
VarDecl *AllocaDecl = CreateAllocaDecl(
getASTContext(), SemaRef.getCurContext(), VarExpr->getBeginLoc(),
@@ -2795,7 +2795,7 @@ SemaOpenACC::CreateFirstPrivateInitRecipe(const Expr *VarExpr) {
// Array sections are special, and we have to treat them that way.
if (const auto *ASE =
dyn_cast<ArraySectionExpr>(VarExpr->IgnoreParenImpCasts()))
- VarTy = ArraySectionExpr::getBaseOriginalType(ASE);
+ VarTy = ASE->getElementType();
VarDecl *AllocaDecl = CreateAllocaDecl(
getASTContext(), SemaRef.getCurContext(), VarExpr->getBeginLoc(),
@@ -2896,7 +2896,7 @@ OpenACCReductionRecipe SemaOpenACC::CreateReductionInitRecipe(
// Array sections are special, and we have to treat them that way.
if (const auto *ASE =
dyn_cast<ArraySectionExpr>(VarExpr->IgnoreParenImpCasts()))
- VarTy = ArraySectionExpr::getBaseOriginalType(ASE);
+ VarTy = ASE->getElementType();
VarDecl *AllocaDecl = CreateAllocaDecl(
getASTContext(), SemaRef.getCurContext(), VarExpr->getBeginLoc(),
diff --git a/clang/lib/Sema/SemaOpenACCClause.cpp b/clang/lib/Sema/SemaOpenACCClause.cpp
index b0869293c1664..e5f443653ec59 100644
--- a/clang/lib/Sema/SemaOpenACCClause.cpp
+++ b/clang/lib/Sema/SemaOpenACCClause.cpp
@@ -1915,48 +1915,31 @@ SemaOpenACC::ActOnClause(ArrayRef<const OpenACCClause *> ExistingClauses,
return Result;
}
-/// OpenACC 3.3 section 2.5.15:
-/// At a mininmum, the supported data types include ... the numerical data types
-/// in C, C++, and Fortran.
-///
-/// If the reduction var is a composite variable, each
-/// member of the composite variable must be a supported datatype for the
-/// reduction operation.
-ExprResult SemaOpenACC::CheckReductionVar(OpenACCDirectiveKind DirectiveKind,
- OpenACCReductionOperator ReductionOp,
- Expr *VarExpr) {
- // For now, we only support 'scalar' types, or composites/arrays of scalar
- // types.
- VarExpr = VarExpr->IgnoreParenCasts();
+bool SemaOpenACC::CheckReductionVarType(Expr *VarExpr) {
SourceLocation VarLoc = VarExpr->getBeginLoc();
SmallVector<PartialDiagnosticAt> Notes;
- QualType CurType = VarExpr->getType();
-
- // For array like things, the expression can either be an array element
- // (subscript expr), array section, or array type. Peel those off, and add
- // notes in case we find an illegal kind. We'll allow scalar or composite of
- // scalars inside of this.
- if (auto *ASE = dyn_cast<ArraySectionExpr>(VarExpr)) {
- QualType BaseType = ArraySectionExpr::getBaseOriginalType(ASE);
+ // The standard isn't clear how many levels of 'array element' or 'subarray'
+ // are permitted, but we can handle as many as we need, so we'll strip them
+ // off here. This will result in CurType being the actual 'type' of the
+ // expression, which is what we are looking to check.
+ QualType CurType = isa<ArraySectionExpr>(VarExpr)
+ ? ArraySectionExpr::getBaseOriginalType(VarExpr)
+ : VarExpr->getType();
+
+ // This can happen when we have a dependent type in an array element that the
+ // above function has tried to 'unwrap'. Since this can only happen with
+ // dependence, just let it go.
+ if (CurType.isNull())
+ return false;
- PartialDiagnostic PD = PDiag(diag::note_acc_reduction_array)
- << diag::OACCReductionArray::Section << BaseType;
- Notes.push_back({ASE->getBeginLoc(), PD});
-
- CurType = getASTContext().getBaseElementType(BaseType);
- } else if (auto *SubExpr = dyn_cast<ArraySubscriptExpr>(VarExpr)) {
- // Array subscript already results in the type of the thing as its type, so
- // there is no type to change here.
- PartialDiagnostic PD =
- PDiag(diag::note_acc_reduction_array)
- << diag::OACCReductionArray::Subscript
- << SubExpr->getBase()->IgnoreParenImpCasts()->getType();
- Notes.push_back({SubExpr->getBeginLoc(), PD});
- } else if (auto *AT = getASTContext().getAsArrayType(CurType)) {
+ // If we are still an array type, we allow 1 level of 'unpeeling' of the
+ // array. The standard isn't clear here whether this is allowed, but
+ // array-of-valid-things makes sense.
+ if (auto *AT = getASTContext().getAsArrayType(CurType)) {
// If we're already the array type, peel off the array and leave the element
// type.
- CurType = getASTContext().getBaseElementType(AT);
+ CurType = AT->getElementType();
PartialDiagnostic PD = PDiag(diag::note_acc_reduction_array)
<< diag::OACCReductionArray::ArrayTy << CurType;
Notes.push_back({VarLoc, PD});
@@ -1974,31 +1957,26 @@ ExprResult SemaOpenACC::CheckReductionVar(OpenACCDirectiveKind DirectiveKind,
for (auto [Loc, PD] : Notes)
Diag(Loc, PD);
- Diag(VarLoc, diag::note_acc_reduction_type_summary);
+ return Diag(VarLoc, diag::note_acc_reduction_type_summary);
};
// If the type is already scalar, or is dependent, just give up.
if (IsValidMemberOfComposite(CurType)) {
// Nothing to do here, is valid.
} else if (auto *RD = CurType->getAsRecordDecl()) {
- if (!RD->isStruct() && !RD->isClass()) {
- EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
- << RD << diag::OACCReductionTy::NotClassStruct);
- return ExprError();
- }
+ if (!RD->isStruct() && !RD->isClass())
+ return EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
+ << RD
+ << diag::OACCReductionTy::NotClassStruct);
- if (!RD->isCompleteDefinition()) {
- EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
- << RD << diag::OACCReductionTy::NotComplete);
- return ExprError();
- }
+ if (!RD->isCompleteDefinition())
+ return EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
+ << RD << diag::OACCReductionTy::NotComplete);
if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
- CXXRD && !CXXRD->isAggregate()) {
- EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
- << CXXRD << diag::OACCReductionTy::NotAgg);
- return ExprError();
- }
+ CXXRD && !CXXRD->isAggregate())
+ return EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
+ << CXXRD << diag::OACCReductionTy::NotAgg);
for (FieldDecl *FD : RD->fields()) {
if (!IsValidMemberOfComposite(FD->getType())) {
@@ -2007,17 +1985,38 @@ ExprResult SemaOpenACC::CheckReductionVar(OpenACCDirectiveKind DirectiveKind,
<< FD->getName() << RD->getName();
Notes.push_back({FD->getBeginLoc(), PD});
// TODO: member here.note_acc_reduction_member_of_composite
- EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
- << FD->getType()
- << diag::OACCReductionTy::MemberNotScalar);
- return ExprError();
+ return EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
+ << FD->getType()
+ << diag::OACCReductionTy::MemberNotScalar);
}
}
} else {
- EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
- << CurType << diag::OACCReductionTy::NotScalar);
+ return EmitDiags(VarLoc, PDiag(diag::err_acc_reduction_type)
+ << CurType
+ << diag::OACCReductionTy::NotScalar);
}
+ return false;
+}
+
+/// OpenACC 3.3 section 2.5.15:
+/// At a mininmum, the supported data types include ... the numerical data types
+/// in C, C++, and Fortran.
+///
+/// If the reduction var is a composite variable, each
+/// member of the composite variable must be a supported datatype for the
+/// reduction operation.
+ExprResult SemaOpenACC::CheckReductionVar(OpenACCDirectiveKind DirectiveKind,
+ OpenACCReductionOperator ReductionOp,
+ Expr *VarExpr) {
+ // For now, we only support 'scalar' types, or composites/arrays of scalar
+ // types.
+ VarExpr = VarExpr->IgnoreParenCasts();
+
+ if (CheckReductionVarType(VarExpr))
+ return ExprError();
+
+
// OpenACC3.3: 2.9.11: Reduction clauses on nested constructs for the same
// reduction 'var' must have the same reduction operator.
if (!VarExpr->isInstantiationDependent()) {
diff --git a/clang/test/SemaOpenACC/combined-construct-reduction-clause.cpp b/clang/test/SemaOpenACC/combined-construct-reduction-clause.cpp
index 5aa90bdc48690..213c940d64561 100644
--- a/clang/test/SemaOpenACC/combined-construct-reduction-clause.cpp
+++ b/clang/test/SemaOpenACC/combined-construct-reduction-clause.cpp
@@ -171,14 +171,12 @@ void uses(unsigned Parm) {
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel loop reduction(+:CoCArr)
for(int i = 0; i < 5; ++i);
- // expected-error@+4{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
- // expected-note@+3{{used as element type of array type 'CompositeHasComposite[5]'}}
+ // expected-error@+3{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
// expected-note@#COS_FIELD{{used as field 'COS' of composite 'CompositeHasComposite'}}
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel loop reduction(+:CoCArr[3])
for(int i = 0; i < 5; ++i);
- // expected-error@+4{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
- // expected-note@+3{{used as element type of sub-array type 'CompositeHasComposite'}}
+ // expected-error@+3{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
// expected-note@#COS_FIELD{{used as field 'COS' of composite 'CompositeHasComposite'}}
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel loop reduction(+:CoCArr[1:1])
diff --git a/clang/test/SemaOpenACC/compute-construct-reduction-clause.c b/clang/test/SemaOpenACC/compute-construct-reduction-clause.c
index 07cb498c55739..265c4986ee135 100644
--- a/clang/test/SemaOpenACC/compute-construct-reduction-clause.c
+++ b/clang/test/SemaOpenACC/compute-construct-reduction-clause.c
@@ -72,8 +72,7 @@ void uses(unsigned Parm) {
while (1);
struct CompositeHasComposite ChCArray[5];
- // expected-error@+4{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
- // expected-note@+3{{used as element type of sub-array type 'struct CompositeHasComposite'}}
+ // expected-error@+3{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
// expected-note@#COS_FIELD{{used as field 'COS' of composite 'CompositeHasComposite'}}
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel reduction(&: CoS, Array[I], ChCArray[0:I])
diff --git a/clang/test/SemaOpenACC/compute-construct-reduction-clause.cpp b/clang/test/SemaOpenACC/compute-construct-reduction-clause.cpp
index 9c2f3d941833f..ddf54f9d76a3f 100644
--- a/clang/test/SemaOpenACC/compute-construct-reduction-clause.cpp
+++ b/clang/test/SemaOpenACC/compute-construct-reduction-clause.cpp
@@ -96,14 +96,12 @@ void uses(unsigned Parm) {
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel reduction(+:CoCArr)
while (1);
- // expected-error@+4{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
- // expected-note@+3{{used as element type of array type 'CompositeHasComposite[5]'}}
+ // expected-error@+3{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
// expected-note@#COS_FIELD{{used as field 'COS' of composite 'CompositeHasComposite'}}
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel reduction(+:CoCArr[3])
while (1);
- // expected-error@+4{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
- // expected-note@+3{{used as element type of sub-array type 'CompositeHasComposite'}}
+ // expected-error@+3{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
// expected-note@#COS_FIELD{{used as field 'COS' of composite 'CompositeHasComposite'}}
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc parallel reduction(+:CoCArr[1:1])
diff --git a/clang/test/SemaOpenACC/loop-construct-reduction-clause.cpp b/clang/test/SemaOpenACC/loop-construct-reduction-clause.cpp
index 2a07c2c19880a..a7440d1ff1dd5 100644
--- a/clang/test/SemaOpenACC/loop-construct-reduction-clause.cpp
+++ b/clang/test/SemaOpenACC/loop-construct-reduction-clause.cpp
@@ -158,14 +158,12 @@ void uses() {
// expected-note@+1{{OpenACC 'reduction' variable reference must be a scalar variable or a composite of scalars, or an array, sub-array, or element of scalar types}}
#pragma acc loop reduction(+:CoCArr)
for(int i = 0; i < 5; ++i);
- // expected-error@+4{{invalid type 'struct CompositeOfScalars' used in OpenACC 'reduction' variable reference; type is not a scalar value}}
- // expected-note@+...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Still going to auto-merge, but found a few things I can use as a test, so stopping it since validation will be finished before i get my tests up :) |
bcardosolopes
approved these changes
Oct 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lately, I've been using 'getBaseOriginalType' in ArraySectionExpr incorrectly: it gets the base-ist of element type, when in reality, I want a single type of indirection. This patch corrects the handful of uses that I had for it.