[ObjC] Check entire chain of superclasses to see if class layout is statically known #81335

AZero13 · 2024-02-09T23:05:10Z

As of now, we only check if a class directly inherits from NSObject to determine if said class has fixed offsets and can therefore "opt-out" from the non-fragile ABI for ivars.

However, if an NSObject subclass has fixed offsets, then so must the subclasses of that subclass, so this allows us to optimize instances of subclasses of subclasses that inherit from NSObject and so on.

To determine this, we need to find that the compiler can see the implementation of each intermediate class, as that means it is statically linked.

Fixes: #81369

llvmbot · 2024-02-09T23:05:42Z

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang-codegen

Author: AtariDreams (AtariDreams)

Changes

If an NSObject subclass has fixed offsets, then so must the subclasses of that subclass!

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGObjCMac.cpp (+12-5)
(modified) clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m (+13)

diff --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp
index 27d77e9a8a5511..ed7b6533752664 100644
--- a/clang/lib/CodeGen/CGObjCMac.cpp
+++ b/clang/lib/CodeGen/CGObjCMac.cpp
@@ -1593,12 +1593,19 @@ class CGObjCNonFragileABIMac : public CGObjCCommonMac {
   }
 
   bool isClassLayoutKnownStatically(const ObjCInterfaceDecl *ID) {
+    if (!ID)
+      return false;
+
     // NSObject is a fixed size. If we can see the @implementation of a class
-    // which inherits from NSObject then we know that all it's offsets also must
-    // be fixed. FIXME: Can we do this if see a chain of super classes with
-    // implementations leading to NSObject?
-    return ID->getImplementation() && ID->getSuperClass() &&
-           ID->getSuperClass()->getName() == "NSObject";
+    // which inherits from NSObject, then we know that all its offsets must
+    // be fixed.
+    if (ID->getImplementation() && ID->getSuperClass() &&
+        ID->getSuperClass()->getName() == "NSObject") {
+      // Check recursively for all intermediate superclasses.
+      return isClassLayoutKnownStatically(ID->getSuperClass());
+    }
+
+    return false;
   }
 
 public:
diff --git a/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m b/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m
index 788b3220af3067..4701f1c5c9b93f 100644
--- a/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m
+++ b/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m
@@ -2,6 +2,7 @@
 
 // CHECK: @"OBJC_IVAR_$_StaticLayout.static_layout_ivar" = hidden constant i64 20
 // CHECK: @"OBJC_IVAR_$_NotStaticLayout.not_static_layout_ivar" = hidden global i64 12
+// CHECK: @"OBJC_IVAR_$_StaticLayoutSubClass.static_layout_ivar2" = hidden global i64 24
 
 @interface NSObject {
   int these, will, never, change, ever;
@@ -20,6 +21,18 @@ -(void)meth {
 }
 @end
 
+@interface StaticLayoutSubClass : StaticLayout
+@end
+
+@implementation StaticLayoutSubClass {
+  int static_layout_ivar2;
+}
+-(void)meth2 {
+  static_layout_ivar2 = 0;
+  // CHECK-NOT: load i64, ptr @"OBJC_IVAR_$_StaticLayoutSubClass.static_layout_ivar2
+}
+@end
+
 @interface NotNSObject {
   int these, might, change;
 }

llvmbot · 2024-02-09T23:05:43Z

@llvm/pr-subscribers-clang

Author: AtariDreams (AtariDreams)

Changes

If an NSObject subclass has fixed offsets, then so must the subclasses of that subclass!

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGObjCMac.cpp (+12-5)
(modified) clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m (+13)

diff --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp
index 27d77e9a8a5511..ed7b6533752664 100644
--- a/clang/lib/CodeGen/CGObjCMac.cpp
+++ b/clang/lib/CodeGen/CGObjCMac.cpp
@@ -1593,12 +1593,19 @@ class CGObjCNonFragileABIMac : public CGObjCCommonMac {
   }
 
   bool isClassLayoutKnownStatically(const ObjCInterfaceDecl *ID) {
+    if (!ID)
+      return false;
+
     // NSObject is a fixed size. If we can see the @implementation of a class
-    // which inherits from NSObject then we know that all it's offsets also must
-    // be fixed. FIXME: Can we do this if see a chain of super classes with
-    // implementations leading to NSObject?
-    return ID->getImplementation() && ID->getSuperClass() &&
-           ID->getSuperClass()->getName() == "NSObject";
+    // which inherits from NSObject, then we know that all its offsets must
+    // be fixed.
+    if (ID->getImplementation() && ID->getSuperClass() &&
+        ID->getSuperClass()->getName() == "NSObject") {
+      // Check recursively for all intermediate superclasses.
+      return isClassLayoutKnownStatically(ID->getSuperClass());
+    }
+
+    return false;
   }
 
 public:
diff --git a/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m b/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m
index 788b3220af3067..4701f1c5c9b93f 100644
--- a/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m
+++ b/clang/test/CodeGenObjC/constant-non-fragile-ivar-offset.m
@@ -2,6 +2,7 @@
 
 // CHECK: @"OBJC_IVAR_$_StaticLayout.static_layout_ivar" = hidden constant i64 20
 // CHECK: @"OBJC_IVAR_$_NotStaticLayout.not_static_layout_ivar" = hidden global i64 12
+// CHECK: @"OBJC_IVAR_$_StaticLayoutSubClass.static_layout_ivar2" = hidden global i64 24
 
 @interface NSObject {
   int these, will, never, change, ever;
@@ -20,6 +21,18 @@ -(void)meth {
 }
 @end
 
+@interface StaticLayoutSubClass : StaticLayout
+@end
+
+@implementation StaticLayoutSubClass {
+  int static_layout_ivar2;
+}
+-(void)meth2 {
+  static_layout_ivar2 = 0;
+  // CHECK-NOT: load i64, ptr @"OBJC_IVAR_$_StaticLayoutSubClass.static_layout_ivar2
+}
+@end
+
 @interface NotNSObject {
   int these, might, change;
 }

github-actions · 2024-02-09T23:06:27Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

rjmccall

Okay, your description of the change here is very misleading; it doesn't even mention that this is specific to having an @implementation for every intermediate class.

Your test seems completely inadequate — you need to test that we're doing the static layout properly, and you need to test with ivars coming from a variety of different places, including:

ivars declared in the @interface
ivars declared in the @implementation
ivars synthesized explicitly in the @implementation
vars synthesized implicitly in the @implementation

AZero13 · 2024-02-10T21:33:36Z

Okay, your description of the change here is very misleading; it doesn't even mention that this is specific to having an @implementation for every intermediate class.

Your test seems completely inadequate — you need to test that we're doing the static layout properly, and you need to test with ivars coming from a variety of different places, including:

ivars declared in the @interface

ivars declared in the @implementation

ivars synthesized explicitly in the @implementation

vars synthesized implicitly in the @implementation

So this is a way to check for static linked; if the compiler can see the @implementation, right?

Only then can we "opt-out" of the non-fragile ABI like NSObject and its subclasses do, right?

AZero13 · 2024-02-10T21:39:19Z

test seems completely inadequate — you need to test that we're doing the static layout properly, and you need to test with ivars coming from a variety of different places, including:

ivars declared in the @interface

ivars declared in the @implementation

ivars synthesized explicitly in the @implementation

vars synthesized implicitly in the @implementation

I think it is the only way, otherwise it would have to be done at link time. Can we do it at link time? probably another PR would have to deal with that.

AZero13 · 2024-02-10T23:59:45Z

Layout before and after my change:

AZero13 · 2024-02-11T00:00:57Z

Sorry, that was codegen, I meant:

AZero13 · 2024-02-26T17:04:53Z

Refactored for code consistency.

rjmccall

Thanks, that looks great.

AZero13 · 2024-02-27T20:21:41Z

@rjmccall Ready to merge!

AZero13 · 2024-02-29T21:51:23Z

@rjmccall I do not have merge permissions or whatever it is called.

AZero13 · 2024-03-03T17:49:36Z

@rjmccall ping

…tatically known As of now, we only check if a class directly inherits from NSObject to determine if said class has fixed offsets and can therefore have its memory layout statically known. However, if an NSObject subclass has fixed offsets, then so must the subclasses of that subclass, so this allows us to optimize instances of subclasses of subclasses that inherit from NSObject and so on. To determine this, we need to find that the compiler can see the implementation of each intermediate class, as that means it is statically linked.

AZero13 · 2024-03-03T20:20:38Z

@JOE1994 Thoughts on this?

JOE1994 · 2024-03-03T20:38:58Z

@JOE1994 Thoughts on this?

I'm not an ideal reviewer for this as I'm unfamiliar with ObjC side of Clang,
but I'll come back to leave a review and to help merge this if this doesn't get merged by the end of today :)

AZero13 · 2024-03-04T01:03:03Z

@JOE1994 Thoughts on this?

I'm not an ideal reviewer for this as I'm unfamiliar with ObjC side of Clang,

but I'll come back to leave a review and to help merge this if this doesn't get merged by the end of today :)

Alright. Thank you so much!

JOE1994

LGTM

…out is statically known (llvm#81335) As of now, we only check if a class directly inherits from NSObject to determine if said class has fixed offsets and can therefore "opt-out" from the non-fragile ABI for ivars. However, if an NSObject subclass has fixed offsets, then so must the subclasses of that subclass, so this allows us to optimize instances of subclasses of subclasses that inherit from NSObject and so on. To determine this, we need to find that the compiler can see the implementation of each intermediate class, as that means it is statically linked. This would be nice to incorporate into the next Swift Toolchain, especially since WebKit, as it is currently configured to be built, would benefit greatly from this.

…tatically known (llvm#81335) As of now, we only check if a class directly inherits from NSObject to determine if said class has fixed offsets and can therefore "opt-out" from the non-fragile ABI for ivars. However, if an NSObject subclass has fixed offsets, then so must the subclasses of that subclass, so this allows us to optimize instances of subclasses of subclasses that inherit from NSObject and so on. To determine this, we need to find that the compiler can see the implementation of each intermediate class, as that means it is statically linked. Fixes: llvm#81369

nocchijiang · 2025-03-10T10:45:27Z

@AZero13 Do you have any news or plans regarding LTO support? We have several projects with substantial existing Objective-C code, and the performance benefits of LTO support could be significant. I would be interested in taking on a portion or the entirety of this work if it is not currently on your priority list.

AZero13 · 2025-03-11T01:52:36Z

@AZero13 Do you have any news or plans regarding LTO support? We have several projects with substantial existing Objective-C code, and the performance benefits of LTO support could be significant. I would be interested in taking on a portion or the entirety of this work if it is not currently on your priority list.

If you can do it, go for it!

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Feb 9, 2024

AZero13 force-pushed the ObjC branch from d10854f to 4aea50d Compare February 9, 2024 23:05

AZero13 force-pushed the ObjC branch 7 times, most recently from e554230 to 2ef8091 Compare February 10, 2024 01:12

AaronBallman requested a review from rjmccall February 10, 2024 13:31

AaronBallman added the objective-c label Feb 10, 2024

AZero13 force-pushed the ObjC branch from 2ef8091 to 193bec6 Compare February 10, 2024 16:22

AZero13 changed the title ~~[ObjC] Recursively check superclasses to see if any inherit from NSObject~~ [ObjC] Check entire chain of superclasses to see if class has fixed offsets Feb 10, 2024

AZero13 force-pushed the ObjC branch 2 times, most recently from 6f5944a to f196660 Compare February 10, 2024 18:28

rjmccall requested changes Feb 10, 2024

View reviewed changes

AZero13 force-pushed the ObjC branch from f196660 to f22d935 Compare February 10, 2024 23:30

AZero13 requested a review from rjmccall February 10, 2024 23:31

AZero13 force-pushed the ObjC branch 4 times, most recently from 3679a6b to 698bb84 Compare February 10, 2024 23:59

AZero13 force-pushed the ObjC branch from 3f36648 to 87cea9f Compare February 24, 2024 05:42

AZero13 changed the title ~~[ObjC] Check entire chain of superclasses to see if class layout can be statically known~~ [ObjC] Check entire chain of superclasses to see if class layout is statically known Feb 24, 2024

AZero13 force-pushed the ObjC branch 3 times, most recently from c23121c to 40fc220 Compare February 26, 2024 16:59

llvmbot added the clang:analysis label Feb 26, 2024

AZero13 force-pushed the ObjC branch 3 times, most recently from a490bdc to c606c0e Compare February 26, 2024 21:57

rjmccall approved these changes Feb 27, 2024

View reviewed changes

AZero13 added 2 commits March 3, 2024 13:45

[ObjC] Add pre-commit tests [NFC]

b6d0f6b

AZero13 force-pushed the ObjC branch from c606c0e to 9b33c9b Compare March 3, 2024 18:45

JOE1994 approved these changes Mar 5, 2024

View reviewed changes

JOE1994 merged commit 923ddf6 into llvm:main Mar 5, 2024

AZero13 deleted the ObjC branch March 5, 2024 18:01

AZero13 restored the ObjC branch March 5, 2024 23:31

AZero13 deleted the ObjC branch March 5, 2024 23:31

[ObjC] Check entire chain of superclasses to see if class layout is statically known #81335

[ObjC] Check entire chain of superclasses to see if class layout is statically known #81335

Uh oh!

Conversation

AZero13 commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

llvmbot commented Feb 9, 2024

Uh oh!

github-actions bot commented Feb 9, 2024

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

AZero13 commented Feb 10, 2024

Uh oh!

AZero13 commented Feb 10, 2024

Uh oh!

AZero13 commented Feb 10, 2024

Uh oh!

AZero13 commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AZero13 commented Feb 26, 2024

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

AZero13 commented Feb 27, 2024

Uh oh!

AZero13 commented Feb 29, 2024

Uh oh!

AZero13 commented Mar 3, 2024

Uh oh!

AZero13 commented Mar 3, 2024

Uh oh!

JOE1994 commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AZero13 commented Mar 4, 2024

Uh oh!

JOE1994 left a comment

Choose a reason for hiding this comment

Uh oh!

nocchijiang commented Mar 10, 2025

Uh oh!

AZero13 commented Mar 11, 2025

Uh oh!

Uh oh!

AZero13 commented Feb 9, 2024 •

edited

Loading

llvmbot commented Feb 9, 2024 •

edited

Loading

AZero13 commented Feb 11, 2024 •

edited

Loading

JOE1994 commented Mar 3, 2024 •

edited

Loading