[Matrix] Preserve signedness when extending matrix index expression. #103044
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Florian Hahn (fhahn) ChangesAs per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed. As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning. [1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator Full diff: https://github.com/llvm/llvm-project/pull/103044.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8defc8e1c185c0..e2c620f0260140 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5021,8 +5021,10 @@ ExprResult Sema::CreateBuiltinMatrixSubscriptExpr(Expr *Base, Expr *RowIdx,
}
}
- ExprResult ConvExpr =
- tryConvertExprToType(IndexExpr, Context.getSizeType());
+ ExprResult ConvExpr = tryConvertExprToType(
+ IndexExpr, IndexExpr->getType()->isSignedIntegerType()
+ ? Context.getSignedSizeType()
+ : Context.getSizeType());
assert(!ConvExpr.isInvalid() &&
"should be able to convert any integer type to size type");
return ConvExpr.get();
diff --git a/clang/test/SemaCXX/matrix-index-operator-sign-conversion.cpp b/clang/test/SemaCXX/matrix-index-operator-sign-conversion.cpp
index 4254780651c5f5..e6fe4a6c57ff22 100644
--- a/clang/test/SemaCXX/matrix-index-operator-sign-conversion.cpp
+++ b/clang/test/SemaCXX/matrix-index-operator-sign-conversion.cpp
@@ -2,19 +2,16 @@
template <typename T, int R, int C> using m __attribute__((__matrix_type__(R,C))) = T;
-// FIXME: should not warn here.
double index1(m<double,3,1> X, int i) { return X[i][0]; }
-// expected-warning@-1 {{implicit conversion changes signedness: 'int' to 'unsigned long'}}
double index2(m<double,3,1> X, unsigned i) { return X[i][0]; }
double index3(m<double,3,1> X, char i) { return X[i][0]; }
-// expected-warning@-1 {{implicit conversion changes signedness: 'char' to 'unsigned long'}}
double index4(m<double,3,1> X, int i) { return X[0][i]; }
-// expected-warning@-1 {{implicit conversion changes signedness: 'int' to 'unsigned long'}}
double index5(m<double,3,1> X, unsigned i) { return X[0][i]; }
double index6(m<double,3,1> X, char i) { return X[0][i]; }
-// expected-warning@-1 {{implicit conversion changes signedness: 'char' to 'unsigned long'}}
+
+// expected-no-diagnostics
|
|
Do you just want whatever the logic is for array subscripts? I believe array subscripts technically leave the index unconverted, and then we perform the necessary conversion when emitting IR. |
fhahn
force-pushed
the
matrix-sign-conversion-index-operator
branch
from
4902d20 to
75a2fd9
Compare
Updated to do it during codegen. Unfortunately it needs to be done in 2 different places it seems. Is this what you had in mind? |
clang/lib/CodeGen/CGExpr.cpp
Outdated
| // Extend or truncate the index type to 32 or 64-bits. | ||
| auto EmitIndex = [this](const Expr *E) { | ||
| llvm::Value *Idx = EmitScalarExpr(E); | ||
| bool IsSigned = E->getType()->isSignedIntegerOrEnumerationType(); |
There was a problem hiding this comment.
You can sink this into the if block.
clang/lib/CodeGen/CGExpr.cpp
Outdated
| llvm::Value *ColIdx = EmitScalarExpr(E->getColumnIdx()); | ||
|
|
||
| // Extend or truncate the index type to 32 or 64-bits. | ||
| auto EmitIndex = [this](const Expr *E) { |
There was a problem hiding this comment.
Can you just pull this out as a helper function on CGF and then call it in both places?
It looks like EmitArraySubscriptExpr does something similar, but it wouldn't be easy to share the logic because it also potentially emits a sanitizer check. On the other hand, is that something we should also be doing here? I assume this is UB if it's out-of-bounds. We always have static bounds on matrix types, right?
There was a problem hiding this comment.
Can you just pull this out as a helper function on CGF and then call it in both places?
Done, thanks
It looks like EmitArraySubscriptExpr does something similar, but it wouldn't be easy to share the logic because it also potentially emits a sanitizer check. On the other hand, is that something we should also be doing here? I assume this is UB if it's out-of-bounds. We always have static bounds on matrix types, right?
yes we have static bounds, so it would probably make sense to extend to logic to also support sanitizer checks for matrix subscript expressions. But probably better done separately?
There was a problem hiding this comment.
Yeah, that would be fine.
As per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed. As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning. [1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator
fhahn
force-pushed
the
matrix-sign-conversion-index-operator
branch
from
75a2fd9 to
e05725e
Compare
…lvm#103044) As per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed. As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning. [1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator PR: llvm#103044
…lvm#103044) As per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed. As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning. [1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator PR: llvm#103044
…lvm#103044) As per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed. As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning. [1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator PR: llvm#103044
* [Matrix] Add test showing unintended implicit sign conversion warning. * [Matrix] Preserve signedness when extending matrix index expression. (llvm#103044) As per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed. As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning. [1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator PR: llvm#103044
As per [1] the indices for a matrix element access operator shall have integral or unscoped enumeration types and be non-negative. At the moment, the index expression is converted to SizeType irrespective of the signedness of the index expression. This causes implicit sign conversion warnings if any of the indices is signed.
As per the spec, using signed types as indices is allowed and should not cause any warnings. If the index expression is signed, extend to SignedSizeType to avoid the warning.
[1] https://clang.llvm.org/docs/MatrixTypes.html#matrix-type-element-access-operator