[clang][ExprConst] allow single element access of vector object to be constant expression #72607

yuanfang-chen · 2023-11-17T03:28:00Z

Supports both v[0] and v.x/v.r/v.s0 syntax.

Selecting multiple elements is left as a future work.

llvmbot · 2023-11-17T03:28:31Z

@llvm/pr-subscribers-clang

Author: Yuanfang Chen (yuanfang-chen)

Changes

Supports both v[0] and v.x/v.r/v.s0 syntax.

Selecting multiple elements is left as a future work.

Full diff: https://github.com/llvm/llvm-project/pull/72607.diff

4 Files Affected:

(modified) clang/lib/AST/ExprConstant.cpp (+95-3)
(modified) clang/lib/AST/Interp/State.h (+2-1)
(modified) clang/test/CodeGenCXX/temporaries.cpp (+21-22)
(added) clang/test/SemaCXX/constexpr-vectors-access-elements.cpp (+29)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index eea0827d6f7a8a1..7468dc5c71fa895 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -436,6 +436,16 @@ namespace {
       MostDerivedArraySize = 2;
       MostDerivedPathLength = Entries.size();
     }
+    void addVectorUnchecked(QualType EltTy, uint64_t Size, uint64_t Idx) {
+      Entries.push_back(PathEntry::ArrayIndex(Idx));
+
+      // This is technically a most-derived object, though in practice this
+      // is unlikely to matter.
+      MostDerivedType = EltTy;
+      MostDerivedIsArrayElement = true;
+      MostDerivedArraySize = Size;
+      MostDerivedPathLength = Entries.size();
+    }
     void diagnoseUnsizedArrayPointerArithmetic(EvalInfo &Info, const Expr *E);
     void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
                                    const APSInt &N);
@@ -1714,6 +1724,11 @@ namespace {
       if (checkSubobject(Info, E, Imag ? CSK_Imag : CSK_Real))
         Designator.addComplexUnchecked(EltTy, Imag);
     }
+    void addVectorElement(EvalInfo &Info, const Expr *E, QualType EltTy,
+                          uint64_t Size, uint64_t Idx) {
+      if (checkSubobject(Info, E, CSK_VectorElement))
+        Designator.addVectorUnchecked(EltTy, Size, Idx);
+    }
     void clearIsNullPointer() {
       IsNullPtr = false;
     }
@@ -3294,6 +3309,19 @@ static bool HandleLValueComplexElement(EvalInfo &Info, const Expr *E,
   return true;
 }
 
+static bool HandleLValueVectorElement(EvalInfo &Info, const Expr *E,
+                                      LValue &LVal, QualType EltTy,
+                                      uint64_t Size, uint64_t Idx) {
+  if (Idx) {
+    CharUnits SizeOfElement;
+    if (!HandleSizeof(Info, E->getExprLoc(), EltTy, SizeOfElement))
+      return false;
+    LVal.Offset += SizeOfElement * Idx;
+  }
+  LVal.addVectorElement(Info, E, EltTy, Size, Idx);
+  return true;
+}
+
 /// Try to evaluate the initializer for a variable declaration.
 ///
 /// \param Info   Information about the ongoing evaluation.
@@ -3839,6 +3867,21 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
         return handler.found(Index ? O->getComplexFloatImag()
                                    : O->getComplexFloatReal(), ObjType);
       }
+    } else if (ObjType->isVectorType()) {
+      uint64_t Index = Sub.Entries[I].getAsArrayIndex();
+      if (Index >= ObjType->castAs<VectorType>()->getNumElements()) {
+        if (Info.getLangOpts().CPlusPlus11)
+          Info.FFDiag(E, diag::note_constexpr_access_past_end)
+            << handler.AccessKind;
+        else
+          Info.FFDiag(E);
+        return handler.failed();
+      }
+
+      ObjType = ObjType->castAs<VectorType>()->getElementType();
+
+      assert(I == N - 1 && "extracting subobject of scalar?");
+      return handler.found(O->getVectorElt(Index), ObjType);
     } else if (const FieldDecl *Field = getAsField(Sub.Entries[I])) {
       if (Field->isMutable() &&
           !Obj.mayAccessMutableMembers(Info, handler.AccessKind)) {
@@ -8294,6 +8337,7 @@ class LValueExprEvaluator
   bool VisitCXXTypeidExpr(const CXXTypeidExpr *E);
   bool VisitCXXUuidofExpr(const CXXUuidofExpr *E);
   bool VisitArraySubscriptExpr(const ArraySubscriptExpr *E);
+  bool VisitExtVectorElementExpr(const ExtVectorElementExpr *E);
   bool VisitUnaryDeref(const UnaryOperator *E);
   bool VisitUnaryReal(const UnaryOperator *E);
   bool VisitUnaryImag(const UnaryOperator *E);
@@ -8607,15 +8651,63 @@ bool LValueExprEvaluator::VisitMemberExpr(const MemberExpr *E) {
   return LValueExprEvaluatorBaseTy::VisitMemberExpr(E);
 }
 
+bool LValueExprEvaluator::VisitExtVectorElementExpr(
+    const ExtVectorElementExpr *E) {
+  bool Success = true;
+
+  APValue Val;
+  if (!Evaluate(Val, Info, E->getBase())) {
+    if (!Info.noteFailure())
+      return false;
+    Success = false;
+  }
+
+  SmallVector<uint32_t, 4> Indices;
+  E->getEncodedElementAccess(Indices);
+  // FIXME: support accessing more than one element
+  if (Indices.size() > 1)
+    return false;
+
+  if (Success) {
+    Result.setFrom(Info.Ctx, Val);
+    const VectorType *VT = E->getBase()->getType()->castAs<VectorType>();
+    HandleLValueVectorElement(Info, E, Result, VT->getElementType(),
+                              VT->getNumElements(), Indices[0]);
+  }
+
+  return Success;
+}
+
 bool LValueExprEvaluator::VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
-  // FIXME: Deal with vectors as array subscript bases.
-  if (E->getBase()->getType()->isVectorType() ||
-      E->getBase()->getType()->isSveVLSBuiltinType())
+  if (E->getBase()->getType()->isSveVLSBuiltinType())
     return Error(E);
 
   APSInt Index;
   bool Success = true;
 
+  if (const VectorType *VT = E->getBase()->getType()->getAs<VectorType>()) {
+    APValue Val;
+    if (!Evaluate(Val, Info, E->getBase())) {
+      if (!Info.noteFailure())
+        return false;
+      Success = false;
+    }
+
+    if (!EvaluateInteger(E->getIdx(), Index, Info)) {
+      if (!Info.noteFailure())
+        return false;
+      Success = false;
+    }
+
+    if (Success) {
+      Result.setFrom(Info.Ctx, Val);
+      HandleLValueVectorElement(Info, E, Result, VT->getElementType(),
+                                VT->getNumElements(), Index.getExtValue());
+    }
+
+    return Success;
+  }
+
   // C++17's rules require us to evaluate the LHS first, regardless of which
   // side is the base.
   for (const Expr *SubExpr : {E->getLHS(), E->getRHS()}) {
diff --git a/clang/lib/AST/Interp/State.h b/clang/lib/AST/Interp/State.h
index f1e8e3618f34fe5..44d6c037c5ad955 100644
--- a/clang/lib/AST/Interp/State.h
+++ b/clang/lib/AST/Interp/State.h
@@ -44,7 +44,8 @@ enum CheckSubobjectKind {
   CSK_ArrayToPointer,
   CSK_ArrayIndex,
   CSK_Real,
-  CSK_Imag
+  CSK_Imag,
+  CSK_VectorElement
 };
 
 namespace interp {
diff --git a/clang/test/CodeGenCXX/temporaries.cpp b/clang/test/CodeGenCXX/temporaries.cpp
index c5adb42a6f17374..135d2a356459272 100644
--- a/clang/test/CodeGenCXX/temporaries.cpp
+++ b/clang/test/CodeGenCXX/temporaries.cpp
@@ -64,6 +64,27 @@ namespace RefTempSubobject {
   constexpr const SelfReferential &sr = SelfReferential();
 }
 
+namespace Vector {
+  typedef __attribute__((vector_size(16))) int vi4a;
+  typedef __attribute__((ext_vector_type(4))) int vi4b;
+  struct S {
+    vi4a v;
+    vi4b w;
+  };
+
+  int &&r = S().v[1];
+  // CHECK: @_ZGRN6Vector1rE_ = internal global i32 0, align 4
+  // CHECK: @_ZN6Vector1rE = constant ptr @_ZGRN6Vector1rE_, align 8
+
+  int &&s = S().w[1];
+  // CHECK: @_ZGRN6Vector1sE_ = internal global i32 0, align 4
+  // CHECK: @_ZN6Vector1sE = constant ptr @_ZGRN6Vector1sE_, align 8
+
+  int &&t = S().w.y;
+  // CHECK: @_ZGRN6Vector1tE_ = internal global i32 0, align 4
+  // CHECK: @_ZN6Vector1tE = constant ptr @_ZGRN6Vector1tE_, align 8
+}
+
 struct A {
   A();
   ~A();
@@ -666,28 +687,6 @@ namespace Bitfield {
   int &&r = S().a;
 }
 
-namespace Vector {
-  typedef __attribute__((vector_size(16))) int vi4a;
-  typedef __attribute__((ext_vector_type(4))) int vi4b;
-  struct S {
-    vi4a v;
-    vi4b w;
-  };
-  // CHECK: alloca
-  // CHECK: extractelement
-  // CHECK: store i32 {{.*}}, ptr @_ZGRN6Vector1rE_
-  // CHECK: store ptr @_ZGRN6Vector1rE_, ptr @_ZN6Vector1rE,
-  int &&r = S().v[1];
-
-  // CHECK: alloca
-  // CHECK: extractelement
-  // CHECK: store i32 {{.*}}, ptr @_ZGRN6Vector1sE_
-  // CHECK: store ptr @_ZGRN6Vector1sE_, ptr @_ZN6Vector1sE,
-  int &&s = S().w[1];
-  // FIXME PR16204: The following code leads to an assertion in Sema.
-  //int &&s = S().w.y;
-}
-
 namespace ImplicitTemporaryCleanup {
   struct A { A(int); ~A(); };
   void g();
diff --git a/clang/test/SemaCXX/constexpr-vectors-access-elements.cpp b/clang/test/SemaCXX/constexpr-vectors-access-elements.cpp
new file mode 100644
index 000000000000000..d31db4c496840e1
--- /dev/null
+++ b/clang/test/SemaCXX/constexpr-vectors-access-elements.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 %s -Wno-uninitialized -std=c++17 -fsyntax-only -verify
+
+namespace Vector {
+
+using TwoIntsVecSize __attribute__((vector_size(8))) = int;
+
+constexpr TwoIntsVecSize a = {1,2};
+static_assert(a[1] == 2);
+static_assert(a[2]); // expected-error {{not an integral constant expression}} expected-note {{read of dereferenced one-past-the-end pointer}}
+
+}
+
+namespace ExtVector {
+
+using FourIntsExtVec __attribute__((ext_vector_type(4))) = int;
+
+constexpr FourIntsExtVec b = {1,2,3,4};
+static_assert(b[0] == 1 && b[1] == 2 && b[2] == 3 && b[3] == 4);
+static_assert(b.s0 == 1 && b.s1 == 2 && b.s2 == 3 && b.s3 == 4);
+static_assert(b.x == 1 && b.y == 2 && b.z == 3 && b.w == 4);
+static_assert(b.r == 1 && b.g == 2 && b.b == 3 && b.a == 4);
+static_assert(b[5]); // expected-error {{not an integral constant expression}} expected-note {{read of dereferenced one-past-the-end pointer}}
+
+// FIXME: support selecting multiple elements
+static_assert(b.lo.lo == 1); // expected-error {{not an integral constant expression}}
+// static_assert(b.lo.lo==1 && b.lo.hi==2 && b.hi.lo == 3 && b.hi.hi == 4);
+// static_assert(b.odd[0]==1 && b.odd[1]==2 && b.even[0] == 3 && b.even[1] == 4);
+
+}

github-actions · 2023-11-17T03:31:19Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 9e1ad3cff6a855fdfdc1d91323e2021726da04ea 471f87e727d71e3984d533eeb9db9ebab40e63ff -- clang/test/SemaCXX/constexpr-vectors-access-elements.cpp clang/lib/AST/ExprConstant.cpp clang/lib/AST/Interp/State.h clang/test/CodeGenCXX/temporaries.cpp

View the diff from clang-format here.

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 9e2fc9b1af..3ebe77ae1f 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3861,7 +3861,7 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
       if (Index >= VT->getNumElements()) {
         if (Info.getLangOpts().CPlusPlus11)
           Info.FFDiag(E, diag::note_constexpr_access_past_end)
-            << handler.AccessKind;
+              << handler.AccessKind;
         else
           Info.FFDiag(E);
         return handler.failed();

yuanfang-chen · 2023-11-22T02:29:59Z

ping?

yuanfang-chen · 2023-12-02T03:41:05Z

ping ..

AaronBallman

You should also add a release note about the changes.

clang/lib/AST/ExprConstant.cpp

… constant expression Supports both v[0] and v.x/v.r/v.s0 syntax. Selecting multiple elements is left as a future work.

yuanfang-chen · 2024-01-15T01:26:44Z

ping?

tbaederr · 2024-01-15T10:18:30Z

This basically looks good to me, but I don't know much about vector types.

sethp

Thanks for looking into this! I was surprised to find that clang didn't support element accesses in a constexpr context, I'm glad you've put the effort in to get it working!

Two minor notes below, otherwise LGTM.

clang/docs/ReleaseNotes.rst

sethp · 2024-01-16T02:29:11Z

clang/lib/AST/ExprConstant.cpp

+      MostDerivedIsArrayElement = true;
+      MostDerivedArraySize = Size;


I'm not sure about this: it seems to indicate that an expression like &V[0] ought to be valid, but it's often not possible to take a pointer to a vector element (i.e. when they're living in a vector register).

That said, I see it's what the Complex type is doing, and I whose elements I think are also not individually addressable, so 🤷

That said, I see it's what the Complex type is doing, and I whose elements I think are also not individually addressable, so 🤷

C23 6.2.5p17: Each complex type has the same representation and alignment requirements as an array type containing exactly two elements of the corresponding real type; the first element is equal to the real part, and the second element to the imaginary part, of the complex number.

CC @jcranmer-intel @arsenm for some other opinions on whether you should be able to address a vector type in a constant expression.

I think accessing individual fields should be OK. Taking the address of a vector element feels wrong, but I haven't thought too deeply about it

Until we have evidence that &V[0] makes sense for vector types, I think we should disallow it. It's easier for us to introduce support once we have a use case than to have to maintain support should someone start relying on this without us intending it as a stable extension.

shafik

This mostly makes sense to me, @AaronBallman does this look good to you?

AaronBallman

Sorry, this fell off my radar for a while. The only concern I have is with accidentally allowing &V[0] to mean something; it would be good to reject that and add a test for that situation. Otherwise, this looks reasonable to me.

AaronBallman · 2024-05-09T14:44:07Z

clang/lib/AST/ExprConstant.cpp

+      MostDerivedIsArrayElement = true;
+      MostDerivedArraySize = Size;


Until we have evidence that &V[0] makes sense for vector types, I think we should disallow it. It's easier for us to introduce support once we have a use case than to have to maintain support should someone start relying on this without us intending it as a stable extension.

vikramRH · 2024-05-14T08:22:39Z

@yuanfang-chen , any plans to continue with this PR ?

vikramRH · 2024-06-17T06:22:44Z

@yuanfang-chen , @AaronBallman, @shafik, are we still actively looking into this ? (I would be willing to commandeer this if its not high on your priority list)

yuanfang-chen · 2024-06-17T06:29:46Z

Hello @vikramRH, please feel free to commandeer this.

vikramRH · 2024-06-18T09:20:25Z

Hello @vikramRH, please feel free to commandeer this.

Thanks @yuanfang-chen. Also, clang already rejects expressions like &V[0] (https://godbolt.org/z/eGcxzGo66), which is also true with constexprs and this PR. What's the specific concern here ?

sethp · 2024-06-18T12:35:03Z

Ah, sorry: my specific question is whether the APValue::LValuePathEntry ought to grow an understanding of vector types rather than re-using the array machinery, as here. It sounds like arrays express a couple of properties vectors don't, so there's potential for the evaluator to want to distinguish an LValuePath that ends in an array element from one that references a vector component.

That said, I don't have a strong sense of whether it's worth doing. "Sema won't build an AST that the evaluator would need to reject" is a fair way to avoid telling the two apart, but seems much harder to me to demonstrate: "none of the ways we tried to get the evaluator to mistreat a vector as an array" is only the same property if we've managed to test exhaustively. For what my non-expert opinion is worth, something like this seems more robust to me:

     void addVectorUnchecked(QualType EltTy, uint64_t Size, uint64_t Idx) {
-      Entries.push_back(PathEntry::ArrayIndex(Idx));
+      Entries.push_back(PathEntry::VectorElement(Idx));

-      // This is technically a most-derived object, though in practice this
-      // is unlikely to matter.
       MostDerivedType = EltTy;
-      MostDerivedIsArrayElement = true;
-      MostDerivedArraySize = Size;

(assuming that's coherent, it's been a few months)

@yuanfang-chen

… constant expression (#101126) This is a slightly updated version of #72607, originally authored by @yuanfang-chen

vikramRH · 2024-08-07T10:48:47Z

closing this, since its handled via #101126

@yuanfang-chen

… constant expression (llvm#101126) This is a slightly updated version of llvm#72607, originally authored by @yuanfang-chen Change-Id: Ifdf143d4d766b076bf3010048ab121932c83bb1e

@yuanfang-chen

… constant expression (llvm#101126) This is a slightly updated version of llvm#72607, originally authored by @yuanfang-chen Change-Id: Id9f94ebb5ce74e853ad9704c97170a128bfd2bc9

yuanfang-chen requested review from shafik and tbaederr November 17, 2023 03:28

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 17, 2023

yuanfang-chen force-pushed the const-vec-elt branch from 26a20b1 to b7d7c5f Compare November 23, 2023 07:18

yuanfang-chen requested a review from AaronBallman November 25, 2023 06:00

shafik requested a review from erichkeane December 2, 2023 05:48

tbaederr requested a review from zygoloid December 2, 2023 06:22

AaronBallman reviewed Dec 6, 2023

View reviewed changes

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

yuanfang-chen mentioned this pull request Dec 27, 2023

[Clang] support vector subscript expressions in constant evaluator (WIP) #76379

Closed

[clang][ExprConst] allow single element access of vector object to be…

471f87e

… constant expression Supports both v[0] and v.x/v.r/v.s0 syntax. Selecting multiple elements is left as a future work.

yuanfang-chen force-pushed the const-vec-elt branch from b7d7c5f to 471f87e Compare January 3, 2024 07:50

sethp reviewed Jan 16, 2024

View reviewed changes

This was referenced Apr 1, 2024

[clang] Constexpr for __builtin_shufflevector and __builtin_convertvector #76615

Merged

Add constexpr support for accessing vector elements #87369

Closed

shafik reviewed Apr 5, 2024

View reviewed changes

AaronBallman reviewed May 9, 2024

View reviewed changes

vikramRH mentioned this pull request Jul 30, 2024

[clang][ExprConst] allow single element access of vector object to be constant expression #101126

Merged

vikramRH added a commit that referenced this pull request Aug 7, 2024

[clang][ExprConst] allow single element access of vector object to be…

7753429

… constant expression (#101126) This is a slightly updated version of #72607, originally authored by @yuanfang-chen

vikramRH closed this Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][ExprConst] allow single element access of vector object to be constant expression #72607

[clang][ExprConst] allow single element access of vector object to be constant expression #72607

yuanfang-chen commented Nov 17, 2023

llvmbot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023 •

edited

Loading

yuanfang-chen commented Nov 22, 2023

yuanfang-chen commented Dec 2, 2023

AaronBallman left a comment

yuanfang-chen commented Jan 15, 2024

tbaederr commented Jan 15, 2024

sethp left a comment

sethp Jan 16, 2024

AaronBallman Jan 16, 2024

arsenm Jan 17, 2024

AaronBallman May 9, 2024

shafik left a comment

AaronBallman left a comment

AaronBallman May 9, 2024

vikramRH commented May 14, 2024

vikramRH commented Jun 17, 2024

yuanfang-chen commented Jun 17, 2024

vikramRH commented Jun 18, 2024

sethp commented Jun 18, 2024

vikramRH commented Aug 7, 2024

		MostDerivedIsArrayElement = true;
		MostDerivedArraySize = Size;

[clang][ExprConst] allow single element access of vector object to be constant expression #72607

[clang][ExprConst] allow single element access of vector object to be constant expression #72607

Conversation

yuanfang-chen commented Nov 17, 2023

llvmbot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023 • edited Loading

yuanfang-chen commented Nov 22, 2023

yuanfang-chen commented Dec 2, 2023

AaronBallman left a comment

Choose a reason for hiding this comment

yuanfang-chen commented Jan 15, 2024

tbaederr commented Jan 15, 2024

sethp left a comment

Choose a reason for hiding this comment

sethp Jan 16, 2024

Choose a reason for hiding this comment

AaronBallman Jan 16, 2024

Choose a reason for hiding this comment

arsenm Jan 17, 2024

Choose a reason for hiding this comment

AaronBallman May 9, 2024

Choose a reason for hiding this comment

shafik left a comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

AaronBallman May 9, 2024

Choose a reason for hiding this comment

vikramRH commented May 14, 2024

vikramRH commented Jun 17, 2024

yuanfang-chen commented Jun 17, 2024

vikramRH commented Jun 18, 2024

sethp commented Jun 18, 2024

vikramRH commented Aug 7, 2024

github-actions bot commented Nov 17, 2023 •

edited

Loading