[PATCH] D14737: Convert some ObjC msgSends to runtime calls

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: include/clang/Basic/LangOptions.def:194
@@ -193,2 +193,3 @@
 LANGOPT(ObjCAutoRefCount , 1, 0, "Objective-C automated reference counting")
+LANGOPT(ObjCConvertMessagesToRuntimeCalls         , 1, 0, "objc_* support for retain/release in the runtime")
 LANGOPT(ObjCWeakRuntime     , 1, 0, "__weak support in the ARC runtime")
----------------
I think this can just be a code-gen option.

Also, please use the description "Replace certain message sends with calls to ObjC runtime entrypoints".

================
Comment at: include/clang/Basic/ObjCRuntime.h:179
@@ +178,3 @@
+  /// the runtime are aware of custom retain/release methods and so can send
+  /// the message if required, but otherwise are able to take a fast-path.
+  bool shouldUseARCFunctionsForRetainRelease() const {
----------------
This isn't really an appropriate comment.  Please use:

  Does this runtime provide ARC entrypoints that are likely to be faster than
  an ordinary message send of the appropriate selector?

  The ARC entrypoints are guaranteed to be equivalent to just sending the
  corresponding message.  If the entrypoint is implemented naively as just a
  message send, using it is a trade-off: it sacrifices a few cycles of overhead
  to save a small amount of code.  However, it's possible for runtimes to detect
  and special-case classes that use "standard" retain/release behavior; if that's
  dynamically a large proportion of all retained objects, using the entrypoint will
  also be faster than using a message send.

  When this method returns true, Clang will turn non-super message sends of
  certain selectors into calls to the correspond entrypoint:
    retain => objc_retain
    release => objc_release

================
Comment at: include/clang/Basic/ObjCRuntime.h:182
@@ +181,3 @@
+    switch (getKind()) {
+    case FragileMacOSX: return false;
+    case MacOSX: return getVersion() >= VersionTuple(10, 10);
----------------
Would you mind asking our runtime folks about the fragile runtime?  We do provide these entrypoints there.

================
Comment at: lib/CodeGen/CGObjCMac.cpp:1871
@@ +1870,3 @@
+  // Call runtime methods directly if we can.
+  if (Method && CGM.getLangOpts().ObjCConvertMessagesToRuntimeCalls) {
+    switch (Method->getMethodFamily()) {
----------------
Hmm.  It would be better if you could implement this in CGObjC instead of CGObjCMac so that it automatically works on all runtimes.

Also, you need to gate this on !IsSuper.  Please add a test for that case.

================
Comment at: lib/CodeGen/CGObjCMac.cpp:1876
@@ +1875,3 @@
+        // are identified as OMF_alloc but we only want to call the runtime for
+        // this version.
+        Selector S = Method->getSelector();
----------------
You really ought to have a separate query on the runtime for whether this is okay, since objc_alloc isn't one of the standard ARC entrypoints.

I'm not sure what that ends up meaning for the code-gen option.  It makes sense to just have one command-line option: we should have the target's default rules for rewriting message sends, there should be a runtime option to suppress it for people who need to (e.g. because they're writing a runtime), end of story.  Maybe what you should do is have the driver pass down the suppression option, if indeed it was provided last, and then have the frontend enable stuff based on the target runtime if the suppression option wasn't given; that is, -fno-objc-X would be the -cc1 flag, not -fobjc-X.

================
Comment at: test/CodeGenObjC/convert-messages-to-runtime-calls.m:38
@@ +37,2 @@
+  [NSObject alloc2];
+}
----------------
You should also test that sending "alloc" with a dynamic receiver work.  This includes something like [self alloc] in a class method. 


http://reviews.llvm.org/D14737