Skip to content

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Mar 10, 2025

By C++1z [expr.add]/6, if the array element type and the pointee type are not similar, behavior is undefined.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 10, 2025

@llvm/pr-subscribers-clang

Author: AZero13 (AZero13)

Changes

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

1 Files Affected:

  • (modified) clang/lib/AST/DeclCXX.cpp (+22-1)
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 7eff776882629..ecba340f0425a 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -1689,7 +1689,7 @@ static NamedDecl* getLambdaCallOperatorHelper(const CXXRecordDecl &RD) {
   //     auto L = [](T) { }; // But A's call operator would want A's here.
   //   }
   //
-  // Walk the call operator’s redecl chain to find the one that belongs
+  // Walk the call operator's redecl chain to find the one that belongs
   // to this module.
   //
   // TODO: We need to fix this properly (see
@@ -2465,6 +2465,27 @@ CXXMethodDecl *CXXMethodDecl::getDevirtualizedMethod(const Expr *Base,
   if (Base->isPRValue() && Base->getType()->isRecordType())
     return this;
 
+  // Handle array subscripts with constant indices when the pointee type is known
+  if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(Base)) {
+    QualType BaseTy = ASE->getBase()->getType();
+    
+    // Check if it's a pointer to a record type
+    if (BaseTy->isPointerType() && 
+        BaseTy->getPointeeType()->isRecordType()) {
+      // For C++17 and later, we can devirtualize array access beyond p[0]
+      // According to [expr.add]/6, if the array element type and the pointee
+      // type are not similar, behavior is undefined, so we can assume they are
+      // the same type
+      const ASTContext &Context = getParent()->getASTContext();
+      const LangOptions &LangOpts = Context.getLangOpts();
+      if (LangOpts.CPlusPlus17 && 
+          ASE->getIdx()->isIntegerConstantExpr(Context)) {
+        // It's a constant index, so it's safe to devirtualize
+        return this;
+      }
+    }
+  }
+
   // If we don't even know what we would call, we can't devirtualize.
   const CXXRecordDecl *BestDynamicDecl = Base->getBestDynamicClassType();
   if (!BestDynamicDecl)

@github-actions
Copy link

github-actions bot commented Mar 10, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@AZero13 AZero13 closed this Mar 10, 2025
@AZero13 AZero13 deleted the milchik branch March 10, 2025 00:12
@AZero13 AZero13 restored the milchik branch March 10, 2025 15:49
@AZero13 AZero13 reopened this Mar 10, 2025
@AZero13 AZero13 changed the title Handle indexes [Clang] Allow devirtualization involving array subscripts with constant indices when the pointee type is known Mar 10, 2025
@AZero13 AZero13 force-pushed the milchik branch 2 times, most recently from 239617e to 0eeb738 Compare March 10, 2025 15:57
…nt indices when the pointee type is known

By C++1z [expr.add]/6, if the array element type and the pointee type are not similar, behavior is undefined.
@AZero13 AZero13 marked this pull request as draft March 10, 2025 17:15
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch is very lacking on details, please elaborate in the commit message (that is, the github summary). Also, this needs a release note.

// FIXME: We can devirtualize this, by C++1z [expr.add]/6 (if the array
// element type and the pointee type are not similar, behavior is undefined).
// CHECK: call void %
// By C++1z [expr.add]/6 (if the array element type and the pointee type
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the comment on 89: Why can we virtualize this but not that one?

// }
//
// Walk the call operators redecl chain to find the one that belongs
// Walk the call operator's redecl chain to find the one that belongs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change.


// Check if it's a pointer to a record type
if (BaseTy->isPointerType() && BaseTy->getPointeeType()->isRecordType()) {
// For C++17 and later, we can devirtualize array access beyond p[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the 'beyond [0]' in that standardeeze, can you better clarify it?

Also, where are you checking for >0 here? It seems like you'd do this in every case? What is making this skip the 0 case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants