ObjcRuntime.h: Add mips64, aarch64, and riscv64 to non-legacy dispatch #76694

hmelder · 2024-01-02T00:49:55Z

This PR updates the list of architectures for which libobjc2 has fast-path objc_msgSend implementations.

github-actions · 2024-01-02T00:50:13Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-02T00:50:40Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Hugo Melder (hmelder)

Changes

This PR updates the list of architectures for which libobjc2 has fast-path objc_msgSend implementations.

Related to: gnustep/libobjc2#261

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

1 Files Affected:

(modified) clang/include/clang/Basic/ObjCRuntime.h (+11-4)

diff --git a/clang/include/clang/Basic/ObjCRuntime.h b/clang/include/clang/Basic/ObjCRuntime.h
index 500b2462f00773..29392ad0a0f577 100644
--- a/clang/include/clang/Basic/ObjCRuntime.h
+++ b/clang/include/clang/Basic/ObjCRuntime.h
@@ -101,10 +101,17 @@ class ObjCRuntime {
     // The GNUstep runtime uses a newer dispatch method by default from
     // version 1.6 onwards
     if (getKind() == GNUstep && getVersion() >= VersionTuple(1, 6)) {
-      if (Arch == llvm::Triple::arm ||
-          Arch == llvm::Triple::x86 ||
-          Arch == llvm::Triple::x86_64)
-        return false;
+      switch (Arch) {
+        case llvm::Triple::arm:
+        case llvm::Triple::x86:
+        case llvm::Triple::x86_64:
+        case llvm::Triple::aarch64:
+        case llvm::Triple::mips64:
+        case llvm::Triple::riscv64:
+          return false;
+        default:
+          return true;
+      }
     }
     else if ((getKind() ==  MacOSX) && isNonFragile() &&
              (getVersion() >= VersionTuple(10, 0)) &&

github-actions · 2024-01-02T00:52:21Z

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

hmelder · 2024-01-02T00:58:29Z

CC @davidchisnall

davidchisnall · 2024-01-02T08:31:34Z

clang/include/clang/Basic/ObjCRuntime.h

-      if (Arch == llvm::Triple::arm ||
-          Arch == llvm::Triple::x86 ||
-          Arch == llvm::Triple::x86_64)
+      switch (Arch) {


The version check above is not quite right for these.

AArch64 support is in 1.9.
RV64 in 2.2.

I think mips64 is correct (it looks like I added the code 9 years ago, which probably lines up with the 1.6 release).

The version check above is not quite right for these.

This was my first approach, but it seems like GNUstep and libobjc2 unit tests are always emitting -fobjc-runtime=gnustep-2.0

So one would need to change this flag in the build configuration everytime libobjc2 gets updated.

I can do the version check and update the libobjc2 README to properly document this. I probably need to rewrite the detection in gnustep-make too -_-

This was my first approach, but it seems like GNUstep and libobjc2 unit tests are always emitting -fobjc-runtime=gnustep-2.0

That's fine. We should bump that in both places to the correct version. It's better than having the compiler emit things that are not supported when targeting a specific version of the runtime.

davidchisnall · 2024-01-02T08:32:39Z

This should also come with a test to make sure that we’re generating objc_msgSend or the lookup functions with the runtime versions after / before the support for these architectures, respectively.

hmelder · 2024-01-02T09:49:10Z

This should also come with a test to make sure that we’re generating objc_msgSend or the lookup functions with the runtime versions after / before the support for these architectures, respectively.

Is libobjc2 included in the LLVM CI? I cannot find the place in the workflow where it is installed.

davidchisnall · 2024-01-02T10:54:20Z

No, clang tests are not executable. This test is the right approximate shape:

llvm-project/clang/test/CodeGenObjC/messages.m

Line 18 in a3e8e86

// CHECK-GNU: call {{.*}} @objc_msg_lookup(

The RUN line invokes clang in a few different configurations (you want to invoke it with the -fobjc-runtime=gnustep-{version} flag and the correct triples and then check that we get either objc_msgSend or objc_msg_lookup, depending on whether objc_msgSend is expected to be supported in that configuration.

hmelder · 2024-01-02T19:37:27Z

It seems like isLegacyDispatchDefaultForArch is only called by the driver, and not when passing -cc1.

llvm-project/clang/lib/Driver/ToolChains/Clang.cpp

Lines 3935 to 3953 in 11ac97c

    
           static void RenderObjCOptions(const ToolChain &TC, const Driver &D, 
        
                                         const llvm::Triple &T, const ArgList &Args, 
        
                                         ObjCRuntime &Runtime, bool InferCovariantReturns, 
        
                                         const InputInfo &Input, ArgStringList &CmdArgs) { 
        
             const llvm::Triple::ArchType Arch = TC.getArch(); 
        
             // -fobjc-dispatch-method is only relevant with the nonfragile-abi, and legacy 
        
             // is the default. Except for deployment target of 10.5, next runtime is 
        
             // always legacy dispatch and -fno-objc-legacy-dispatch gets ignored silently. 
        
             if (Runtime.isNonFragile()) { 
        
               if (!Args.hasFlag(options::OPT_fobjc_legacy_dispatch, 
        
                                 options::OPT_fno_objc_legacy_dispatch, 
        
                                 Runtime.isLegacyDispatchDefaultForArch(Arch))) { 
        
                 if (TC.UseObjCMixedDispatch()) 
        
                   CmdArgs.push_back("-fobjc-dispatch-method=mixed"); 
        
                 else 
        
                   CmdArgs.push_back("-fobjc-dispatch-method=non-legacy"); 
        
               } 
        
             }

Using the driver instead of the frontend does not work as there are platform checks in Clang::AddObjCRuntimeArgs.

Example on macOS:

clang: error: GNUstep Objective-C runtime version 2 incompatible with target binary format

The result is that without specifying -fobjc-dispatch-method=mixed or -fobjc-dispatch-method=no-legacy, the frontend will always emit call ptr @objc_msg_lookup_sender.

Do you have an idea on how we can test this instead?

Tested with

#flags="-fobjc-dispatch-method=mixed"
flags=""
rt=gnustep-2.0
triple="x86_64-unknown-freebsd"

build-llvm/bin/clang -cc1\
		-triple ${triple} \
		-fobjc-runtime=${rt} ${flags} \
		-emit-llvm -o - gnustep2-message-dispatch.m | less

gnustep2-message-dispatch.m

// DEFINE: %{triple} =
// DEFINE: %{ver} =
// DEFINE: %{prefix} = CHECK-MSG-SEND
// DEFINE: %{check} = %clang_cc1 -triple %{triple} -fobjc-runtime=gnustep-%{ver} -emit-llvm -o - %s | FileCheck %s -check-prefix %{prefix}


// REDEFINE: %{ver} = 1.6
// REDEFINE: %{triple} = x86_64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = arm-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{prefix} = CHECK-MSG-LOOKUP
// REDEFINE: %{triple} = aarch64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = mips64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = riscv64-unknown-freebsd
// RUN: %{check}

// REDEFINE: %{ver} = 1.9
// REDEFINE: %{prefix} = CHECK-MSG-SEND
// REDEFINE: %{triple} = x86_64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = arm-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = aarch64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = mips64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{prefix} = CHECK-MSG-LOOKUP
// REDEFINE: %{triple} = riscv64-unknown-freebsd
// RUN: %{check}

// REDEFINE: %{ver} = 2.2
// REDEFINE: %{prefix} = CHECK-MSG-SEND
// REDEFINE: %{triple} = x86_64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = arm-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = aarch64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = mips64-unknown-freebsd
// RUN: %{check}
// REDEFINE: %{triple} = riscv64-unknown-freebsd
// RUN: %{check}

typedef struct {
  int x;
  int y;
  int z[10];
} MyPoint;

void f0(id a) {
  int i;
  MyPoint pt = { 1, 2};

  // CHECK-MSG-SEND: call {{.*}} @objc_msgSend
  // CHECK-MSG-LOOKUP: call {{.*}} @objc_msg_lookup(
  [a print0];

  // CHECK-MSG-SEND: call {{.*}} @objc_msgSend
  // CHECK-MSG-LOOKUP: call {{.*}} @objc_msg_lookup(
  [a print1: 10];

  // CHECK-MSG-SEND: call {{.*}} @objc_msgSend
  // CHECK-MSG-LOOKUP: call {{.*}} @objc_msg_lookup(
  [a print2: 10 and: "hello" and: 2.2];

  // CHECK-MSG-SEND: call {{.*}} @objc_msgSend
  // CHECK-MSG-LOOKUP: call {{.*}} @objc_msg_lookup(
  [a takeStruct: pt ];
}

hmelder · 2024-01-02T19:39:42Z

I assume we can split the test into two parts:

Checking if the driver correctly sets -fobjc-dispatch-method
Checking if frontend emits objc_msgSend

hmelder · 2024-01-02T19:43:55Z

Definition of -fobjc-dispatch-method:

def fobjc_dispatch_method_EQ : Joined<["-"], "fobjc-dispatch-method=">,
  HelpText<"Objective-C dispatch method to use">,
  Values<"legacy,non-legacy,mixed">,
  NormalizedValuesScope<"CodeGenOptions">, NormalizedValues<["Legacy", "NonLegacy", "Mixed"]>,
  MarshallingInfoEnum<CodeGenOpts<"ObjCDispatchMethod">, "Legacy">;

Which is used in CGObjCGNU::GenerateMessageSend

davidchisnall · 2024-01-02T21:06:55Z

I assume we can split the test into two parts:

Checking if the driver correctly sets -fobjc-dispatch-method

Checking if frontend emits objc_msgSend

Ah, I forgot that was how it propagated. I think we only need the driver tests, there should be existing checks for the dispatch-method thing.

hmelder · 2024-01-02T21:09:13Z

Should be ready now!

davidchisnall · 2024-01-02T21:09:13Z

A lot of formatting changes seem to have crept into unrelated bits of the code, please can you revert them? You can use the clang-format-diff script to format only the bits that you’ve changed.

This reverts commit 7c69da7.

hmelder · 2024-01-02T21:23:58Z

A lot of formatting changes seem to have crept into unrelated bits of the code, please can you revert them? You can use > the clang-format-diff script to format only the bits that you’ve changed.

Done. Sorry for the messy commit history.

davidchisnall · 2024-01-03T08:17:31Z

LGTM. @rjmccall, do you want to take a look?

rjmccall

LGTM if @davidchisnall's okay with it.

Forces Clang to use the fast path, which is not enabled by default on arm64 even though it's supported in the runtime. Can be removed when Clang 18 containing this patch is available via the Android SDK: llvm/llvm-project#76694

Add mips64, aarch64, and riscv64 to ObjcRuntime dispatch

97a9a27

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 2, 2024

ObjcRuntime.h: Adjust formatting

c9dd0b7

davidchisnall reviewed Jan 2, 2024

View reviewed changes

davidchisnall self-assigned this Jan 2, 2024

davidchisnall added the objective-c label Jan 2, 2024

hmelder added 2 commits January 2, 2024 21:41

ObjCRuntime: format and update dispatch archs

7c69da7

Clang driver dispatch-method test for gnustep runtime

5d2ee5e

llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Jan 2, 2024

hmelder added 2 commits January 2, 2024 21:48

gnustep-dispatch-method: Add trailing space

4a67b48

CodeGenObjC: Add simple non-legacy dispatch gnustep test

a10dbee

hmelder requested a review from davidchisnall January 2, 2024 21:09

hmelder added 2 commits January 2, 2024 22:19

Revert "ObjCRuntime: format and update dispatch archs"

c8cc84a

This reverts commit 7c69da7.

ObjCRuntime: Update gnustep dispatch archs with versioning

9e77495

ObjCRuntime: fix formatting

73cace5

rjmccall approved these changes Jan 3, 2024

View reviewed changes

davidchisnall approved these changes Jan 3, 2024

View reviewed changes

davidchisnall merged commit 3f9f8ef into llvm:main Jan 3, 2024

hmelder mentioned this pull request Oct 21, 2024

FastPathAlloc tests failing on Android NDK r27 gnustep/libobjc2#301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjcRuntime.h: Add mips64, aarch64, and riscv64 to non-legacy dispatch #76694

ObjcRuntime.h: Add mips64, aarch64, and riscv64 to non-legacy dispatch #76694

hmelder commented Jan 2, 2024

github-actions bot commented Jan 2, 2024

llvmbot commented Jan 2, 2024 •

edited

Loading

github-actions bot commented Jan 2, 2024 •

edited

Loading

hmelder commented Jan 2, 2024

davidchisnall Jan 2, 2024

hmelder Jan 2, 2024

hmelder Jan 2, 2024 •

edited

Loading

davidchisnall Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

hmelder commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 3, 2024

rjmccall left a comment

ObjcRuntime.h: Add mips64, aarch64, and riscv64 to non-legacy dispatch #76694

ObjcRuntime.h: Add mips64, aarch64, and riscv64 to non-legacy dispatch #76694

Conversation

hmelder commented Jan 2, 2024

github-actions bot commented Jan 2, 2024

llvmbot commented Jan 2, 2024 • edited Loading

github-actions bot commented Jan 2, 2024 • edited Loading

hmelder commented Jan 2, 2024

davidchisnall Jan 2, 2024

Choose a reason for hiding this comment

hmelder Jan 2, 2024

Choose a reason for hiding this comment

hmelder Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

davidchisnall Jan 2, 2024

Choose a reason for hiding this comment

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

hmelder commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 2, 2024

hmelder commented Jan 2, 2024

davidchisnall commented Jan 3, 2024

rjmccall left a comment

Choose a reason for hiding this comment

llvmbot commented Jan 2, 2024 •

edited

Loading

github-actions bot commented Jan 2, 2024 •

edited

Loading

hmelder Jan 2, 2024 •

edited

Loading