[clang] Return larger CXX records in memory #120670

pranavk · 2024-12-20T02:22:06Z

We incorrectly return CXX records in AVX registers when they should be returned in memory. This is violation of x86-64 psABI.

Detailed discussion is here: https://groups.google.com/g/x86-64-abi/c/BjOOyihHuqg/m/KurXdUcWAgAJ

llvmbot · 2024-12-20T02:22:38Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-backend-x86

Author: Pranav Kant (pranavk)

Changes

We incorrectly return CXX records in AVX registers when they should be returned in memory. This is violation of x86-64 psABI.

Detailed discussion is here: https://groups.google.com/g/x86-64-abi/c/BjOOyihHuqg/m/KurXdUcWAgAJ

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

2 Files Affected:

(modified) clang/include/clang/Basic/LangOptions.h (+1)
(modified) clang/lib/CodeGen/Targets/X86.cpp (+18)

diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index 949c8f5d448bcf..0dd644eba559b9 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -245,6 +245,7 @@ class LangOptionsBase {
     ///   construction vtable because it hasn't added 'type' as a substitution.
     ///   - Skip mangling enclosing class templates of member-like friend
     ///   function templates.
+    ///   - Incorrectly return C++ records in AVX registers.
     Ver19,
 
     /// Conform to the underlying platform's C and C++ ABIs as closely
diff --git a/clang/lib/CodeGen/Targets/X86.cpp b/clang/lib/CodeGen/Targets/X86.cpp
index 7f73bf2a65266e..70d812057d0b01 100644
--- a/clang/lib/CodeGen/Targets/X86.cpp
+++ b/clang/lib/CodeGen/Targets/X86.cpp
@@ -1334,6 +1334,20 @@ class X86_64ABIInfo : public ABIInfo {
     return T.isOSLinux() || T.isOSNetBSD();
   }
 
+  bool returnCXXRecordGreaterThan128InMem(unsigned Size, unsigned TypeSize,
+                                          unsigned NativeSize) const {
+    // Clang <= 19.0 did not do this.
+    if (getContext().getLangOpts().getClangABICompat() <=
+        LangOptions::ClangABI::Ver19)
+      return false;
+
+    // The only case a 256(or 512)-bit wide vector could be used to return
+    // is when CXX record contains a single 256(or 512)-bit element.
+    if (Size > 128 && (Size != TypeSize || Size > NativeSize))
+      return true;
+    return false;
+  }
+
   X86AVXABILevel AVXLevel;
   // Some ABIs (e.g. X32 ABI and Native Client OS) use 32 bit pointers on
   // 64-bit hardware.
@@ -2067,6 +2081,10 @@ void X86_64ABIInfo::classify(QualType Ty, uint64_t OffsetBase, Class &Lo,
         classify(I.getType(), Offset, FieldLo, FieldHi, isNamedArg);
         Lo = merge(Lo, FieldLo);
         Hi = merge(Hi, FieldHi);
+        if (returnCXXRecordGreaterThan128InMem(
+                Size, getContext().getTypeSize(I.getType()),
+                getNativeVectorSizeForAVXABI(AVXLevel)))
+          Lo = Memory;
         if (Lo == Memory || Hi == Memory) {
           postMerge(Size, Lo, Hi);
           return;

efriedma-quic · 2024-12-20T18:20:35Z

Missing regression test in clang/test/CodeGen.

We probably want a release note for this (clang/docs/ReleaseNotes.rst).

(If you're interested, there's another x86-64 ABI bug which nobody got around to fixing: #76017.)

phoebewang · 2024-12-21T14:57:44Z

clang/include/clang/Basic/LangOptions.h

@@ -245,6 +245,7 @@ class LangOptionsBase {
    ///   construction vtable because it hasn't added 'type' as a substitution.
    ///   - Skip mangling enclosing class templates of member-like friend
    ///   function templates.
+    ///   - Incorrectly return C++ records in AVX registers.


in AVX registers on x86_64.

phoebewang · 2024-12-31T00:32:36Z

clang/lib/CodeGen/Targets/X86.cpp

+        if (returnCXXRecordGreaterThan128InMem(
+                Size, getContext().getTypeSize(I.getType()),
+                getNativeVectorSizeForAVXABI(AVXLevel)))
+          Lo = Memory;


Why don't do early check and return like line 2022? IIRC, the default value of Lo/Hi is Memory.

This seemed like the most appropriate location for such a check as I only want to do it for CXXRecords (if dyn_cast<CXXRecordDecl> ...) which is already being checked here.

phoebewang

LGTM.

phoebewang · 2025-01-03T02:09:17Z

clang/test/CodeGen/X86/avx-cxx-record.cpp

@@ -0,0 +1,23 @@
+// RUN: %clang %s -S --target=x86_64-unknown-linux-gnu -emit-llvm -O2 -march=x86-64-v3 -o - | FileCheck %s


Add a RUN line for BSD.

shafik

Shouldn't this have some sort of Release not maybe under the X86 section?

pranavk · 2025-01-24T23:06:35Z

Shouldn't this have some sort of Release not maybe under the X86 section?

you mean mention this in release section? I plan to add a note in clang/docs/ReleaseNotes.rst. Next week.

pranavk · 2025-01-28T22:51:31Z

Shouldn't this have some sort of Release not maybe under the X86 section?

there's no x86 section in the release notes. So I added it in ABI section.

phoebewang · 2025-01-29T06:36:37Z

Shouldn't this have some sort of Release not maybe under the X86 section?

there's no x86 section in the release notes. So I added it in ABI section.

There is a X86 Support section. Note, the ReleaseNotes was updated since LLVM 20 branch created. You need to merge the main and update it.

We incorrectly return CXX records in AVX registers when they should be returned in memory. This is violation of x86-64 psABI. Detailed discussion is here: https://groups.google.com/g/x86-64-abi/c/BjOOyihHuqg/m/KurXdUcWAgAJ

pranavk · 2025-01-29T17:38:06Z

Oops. Too bad. I didn't realize we branched off release/20.x

Anyhow, I made necessary tweaks to now make it a LLVM 21 feature.

There is a X86 Support section

It feels more appropriate to put this in "ABI changes section" than "X86 support" since it's not a "support". But I am happy to move that over if that's the precedence for these kind of changes. Let me know.

efriedma-quic · 2025-01-29T22:07:24Z

You can cherry-pick this to the 20.x branch if you want; we can still land fixes like this for a few weeks after the branch is created.

pranavk · 2025-01-29T23:20:35Z

We don't need this urgently. I am fine just keeping it like this.

efriedma-quic · 2025-01-30T17:49:30Z

clang/lib/CodeGen/Targets/X86.cpp

+      return false;
+
+    const llvm::Triple &T = getTarget().getTriple();
+    return T.isOSLinux() || T.isOSNetBSD();


Please make this apply to all targets by default. Almost all targets will want to be consistent with the spec and gcc. If you think you need to exclude some target for some reason, please list it explicitly.

kazutakahirata · 2025-02-04T18:38:23Z

I've checked in 560e372 to fix the build.

pranavk · 2025-02-04T18:57:18Z

Thank you

dyung · 2025-02-04T23:02:06Z

@pranavk the test you added seems to fail in a release build without asserts enabled. Can you take a look?

https://lab.llvm.org/staging/#/builders/202/builds/857/steps/6/logs/FAIL__Clang__avx-cxx-record_cpp

pranavk · 2025-02-04T23:10:22Z

Checking ...

pranavk · 2025-02-05T00:13:10Z

I fixed this in #125787

We incorrectly return CXX records in AVX registers when they should be returned in memory. This is violation of x86-64 psABI. Detailed discussion is here: https://groups.google.com/g/x86-64-abi/c/BjOOyihHuqg/m/KurXdUcWAgAJ

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 20, 2024

efriedma-quic requested review from phoebewang, CoTinker and efriedma-quic December 20, 2024 18:18

phoebewang reviewed Dec 21, 2024

View reviewed changes

pranavk force-pushed the psabi branch from 573cb26 to 6e62d01 Compare December 30, 2024 14:58

phoebewang reviewed Dec 31, 2024

View reviewed changes

pranavk force-pushed the psabi branch from 73f9f5c to 852907e Compare January 3, 2025 00:43

phoebewang approved these changes Jan 3, 2025

View reviewed changes

shafik reviewed Jan 24, 2025

View reviewed changes

pranavk force-pushed the psabi branch from 852907e to 403bcdd Compare January 28, 2025 22:49

pranavk force-pushed the psabi branch from 403bcdd to b729ea2 Compare January 29, 2025 17:27

[clang] Return larger CXX records in memory

2c049c1

We incorrectly return CXX records in AVX registers when they should be returned in memory. This is violation of x86-64 psABI. Detailed discussion is here: https://groups.google.com/g/x86-64-abi/c/BjOOyihHuqg/m/KurXdUcWAgAJ

pranavk force-pushed the psabi branch from b729ea2 to 2c049c1 Compare January 29, 2025 17:36

efriedma-quic reviewed Jan 30, 2025

View reviewed changes

ABI change for all targets

4fe3dda

pranavk merged commit e8a486e into llvm:main Feb 4, 2025
9 checks passed

		@@ -0,0 +1,23 @@
		// RUN: %clang %s -S --target=x86_64-unknown-linux-gnu -emit-llvm -O2 -march=x86-64-v3 -o - \| FileCheck %s

[clang] Return larger CXX records in memory #120670

[clang] Return larger CXX records in memory #120670

Uh oh!

Conversation

pranavk commented Dec 20, 2024

Uh oh!

llvmbot commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic commented Dec 20, 2024

Uh oh!

phoebewang Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

phoebewang Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

pranavk Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

phoebewang left a comment

Choose a reason for hiding this comment

Uh oh!

phoebewang Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

pranavk commented Jan 24, 2025

Uh oh!

pranavk commented Jan 28, 2025

Uh oh!

phoebewang commented Jan 29, 2025

Uh oh!

pranavk commented Jan 29, 2025

Uh oh!

efriedma-quic commented Jan 29, 2025

Uh oh!

pranavk commented Jan 29, 2025

Uh oh!

efriedma-quic Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

pranavk Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kazutakahirata commented Feb 4, 2025

Uh oh!

pranavk commented Feb 4, 2025

Uh oh!

dyung commented Feb 4, 2025

Uh oh!

pranavk commented Feb 4, 2025

Uh oh!

pranavk commented Feb 5, 2025

Uh oh!

Uh oh!

llvmbot commented Dec 20, 2024 •

edited

Loading