[PATCH] D46052: GNUstep Objective-C ABI version 2

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, the comments help a lot.



================
Comment at: lib/CodeGen/CGObjCGNU.cpp:439
+                             ArrayRef<llvm::Constant *> IvarOffsets,
+                             ArrayRef<llvm::Constant *> IvarAlign,
+                             ArrayRef<Qualifiers::ObjCLifetime> IvarOwnership);
----------------
theraven wrote:
> DHowett-MSFT wrote:
> > While we're here, is there value in storing the ivar size in layout as well, so that the runtime doesn't need to calculate it from the distance to the next ivar/end of the instance?
> Normally the runtime calculates it from the type encoding, if it's required.  I agree that it might be nice to have though.  Do you have strong feelings about needing it in the 2.0 ABI?  The looser coupling means that it would be easy to add in the 2.1 ABI if we want it later.  Are you seeing cases where the runtime is calculating it incorrectly because of insufficient information in the type encoding, or where calculating it is causing performance problems?
The distance between ivar offsets wouldn't be correct anyway because of alignment padding.  A set-ivar function might be able to get away with copying too many bytes from an input buffer (although I wouldn't recommend it!), but a get-ivar function definitely should not copy too many bytes into an output buffer.

And all that's assuming that the runtime promises not to reorder ivars dynamically.  I don't know what the GNU runtime says about that.  (Static reordering is fine, since the runtime can reasonably demand that the compiler emit ivars in layout order.)


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:1065
+      }
+    }
+    auto *ObjCStrGV =
----------------
Seems much more reasonable, thanks.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:1067
+    auto *ObjCStrGV =
+      Fields.finishAndCreateGlobal(isNamed ? StringName : ".objc_string",
+          Align, false, isNamed ? llvm::GlobalValue::LinkOnceODRLinkage :
----------------
Micro-optimization: I'm pretty sure the type of this ternary is an r-value `std::string`, which means copying the string in either case; if you explicitly build a StringRef from StringName, the ternary will instead construct a StringRef and you'll avoid those unnecessary copies.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:1069
+          Align, false, isNamed ? llvm::GlobalValue::LinkOnceODRLinkage :
+          llvm::GlobalValue::PrivateLinkage);
+    ObjCStrGV->setSection(ConstantStringSection);
----------------
The indentation here is a little misleading; I'd suggest aligning the two cases of the ternary operator.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:1172
+    if (isWeak) {
+      // Placeholder for the real symbol.
+      ClassSymbol->setInitializer(new llvm::GlobalVariable(TheModule,
----------------
I would suggest clarifying in what sense this is a placeholder.  Does the runtime recognize it specially?  If so, how?  Is it replaced statically by a later pass in IRGen?


Repository:
  rC Clang

https://reviews.llvm.org/D46052