[clang] [Matrix] Preserve signedness when extending matrix index expression. (PR #103044)

[Florian Hahn via cfe-commits]

https://github.com/fhahn updated https://github.com/llvm/llvm-project/pull/103044

>From b68f638ad8b8a1c217d94d349070bc59ab531a0e Mon Sep 17 00:00:00 2001
From: Florian Hahn <flo at fhahn.com>
Date: Tue, 13 Aug 2024 12:28:34 +0100
Subject: [PATCH 1/2] [Matrix] Preserve signedness when extending matrix index
 expression.

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
---
 clang/lib/Sema/SemaExpr.cpp                                | 6 ++++--
 .../test/SemaCXX/matrix-index-operator-sign-conversion.cpp | 7 ++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 0f58eb2840211d..258e931ef592e5 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5025,8 +5025,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 at -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 at -1 {{implicit conversion changes signedness: 'char' to 'unsigned long'}}
 
 double index4(m<double,3,1> X, int      i) { return X[0][i]; }
-// expected-warning at -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 at -1 {{implicit conversion changes signedness: 'char' to 'unsigned long'}}
+
+// expected-no-diagnostics

>From 75a2fd9e49897b6d243c3580e5b9a7f2361a9d39 Mon Sep 17 00:00:00 2001
From: Florian Hahn <flo at fhahn.com>
Date: Thu, 15 Aug 2024 17:29:42 +0100
Subject: [PATCH 2/2] !fixup move extends to CG.

---
 clang/lib/CodeGen/CGExpr.cpp       | 14 ++++++++++++--
 clang/lib/CodeGen/CGExprScalar.cpp | 11 +++++++++--
 clang/lib/Sema/SemaExpr.cpp        |  5 +----
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 0672861790633b..00b8eb43a663ae 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4348,8 +4348,18 @@ LValue CodeGenFunction::EmitMatrixSubscriptExpr(const MatrixSubscriptExpr *E) {
       !E->isIncomplete() &&
       "incomplete matrix subscript expressions should be rejected during Sema");
   LValue Base = EmitLValue(E->getBase());
-  llvm::Value *RowIdx = EmitScalarExpr(E->getRowIdx());
-  llvm::Value *ColIdx = EmitScalarExpr(E->getColumnIdx());
+
+  // 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();
+    if (Idx->getType() != IntPtrTy)
+      Idx = Builder.CreateIntCast(Idx, IntPtrTy, IsSigned);
+    return Idx;
+  };
+  llvm::Value *RowIdx = EmitIndex(E->getRowIdx());
+  llvm::Value *ColIdx = EmitIndex(E->getColumnIdx());
+
   llvm::Value *NumRows = Builder.getIntN(
       RowIdx->getType()->getScalarSizeInBits(),
       E->getBase()->getType()->castAs<ConstantMatrixType>()->getNumRows());
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 6eac2b4c54e1ba..2c935e431563cb 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2007,8 +2007,15 @@ Value *ScalarExprEmitter::VisitMatrixSubscriptExpr(MatrixSubscriptExpr *E) {
 
   // Handle the vector case.  The base must be a vector, the index must be an
   // integer value.
-  Value *RowIdx = Visit(E->getRowIdx());
-  Value *ColumnIdx = Visit(E->getColumnIdx());
+  auto VisitIndex = [this](Expr *E) {
+    llvm::Value *Idx = Visit(E);
+    bool IsSigned = E->getType()->isSignedIntegerOrEnumerationType();
+    if (Idx->getType() != CGF.IntPtrTy)
+      Idx = Builder.CreateIntCast(Idx, CGF.IntPtrTy, IsSigned);
+    return Idx;
+  };
+  Value *RowIdx = VisitIndex(E->getRowIdx());
+  Value *ColumnIdx = VisitIndex(E->getColumnIdx());
 
   const auto *MatrixTy = E->getBase()->getType()->castAs<ConstantMatrixType>();
   unsigned NumRows = MatrixTy->getNumRows();
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 258e931ef592e5..80442cb25d0ab6 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5025,10 +5025,7 @@ ExprResult Sema::CreateBuiltinMatrixSubscriptExpr(Expr *Base, Expr *RowIdx,
       }
     }
 
-    ExprResult ConvExpr = tryConvertExprToType(
-        IndexExpr, IndexExpr->getType()->isSignedIntegerType()
-                       ? Context.getSignedSizeType()
-                       : Context.getSizeType());
+    ExprResult ConvExpr = IndexExpr;
     assert(!ConvExpr.isInvalid() &&
            "should be able to convert any integer type to size type");
     return ConvExpr.get();