Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YJIT: A64: Use ADDS/SUBS/CMP (immediate) when possible #10402

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Mar 28, 2024

We were loading 1 into a register and then doing ADDS/SUBS previously.
That was particularly bad since those come up in fixnum operations.

   # integer left shift with rhs=1
-  mov x11, #1
-  subs x11, x1, x11
+  subs x11, x1, #1
   lsl x12, x11, #1
   asr x13, x12, #1
   cmp x13, x11
-  b.ne #0x106ab60f8
-  mov x11, #1
-  adds x12, x12, x11
+  b.ne #0x10903a0f8
+  adds x12, x12, #1
   mov x1, x12

@XrXr XrXr requested a review from a team as a code owner March 28, 2024 21:48
@XrXr XrXr marked this pull request as draft March 28, 2024 21:51

This comment has been minimized.

We were loading 1 into a register and then doing ADDS/SUBS previously.
That was particularly bad since those come up in fixnum operations.

```diff
   # integer left shift with rhs=1
-  mov x11, #1
-  subs x11, x1, x11
+  subs x11, x1, #1
   lsl x12, x11, #1
   asr x13, x12, #1
   cmp x13, x11
-  b.ne #0x106ab60f8
-  mov x11, #1
-  adds x12, x12, x11
+  b.ne #0x10903a0f8
+  adds x12, x12, #1
   mov x1, x12
```

Note that it's fine to cast between i64 and u64 since the bit pattern is
preserved, and the add/sub themselves don't care about the signedness of
the operands.

CMP is just another mnemonic for SUBS.
Copy link
Contributor

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

Copy link
Contributor

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to make sure every instruction it's passed to will handle this signature now.

@XrXr XrXr changed the title YJIT: A64: Use ADDS (immediate) when possible YJIT: A64: Use ADDS/SUBS/CMP (immediate) when possible Mar 28, 2024
There is in fact no MUL on A64 that takes an immediate, so this
instruction was using the wrong split method. No current usages of this
form in YJIT.
@XrXr XrXr marked this pull request as ready for review March 28, 2024 22:33
@maximecb
Copy link
Contributor

maximecb commented Apr 2, 2024

Good find. There were some failing tests that don't seem related to this PR. Updating the branch to run the tests again.

@maximecb maximecb merged commit 3c4de94 into ruby:master Apr 2, 2024
99 checks passed
artur-intech pushed a commit to artur-intech/ruby that referenced this pull request Apr 26, 2024
* YJIT: A64: Use ADDS/SUBS/CMP (immediate) when possible

We were loading 1 into a register and then doing ADDS/SUBS previously.
That was particularly bad since those come up in fixnum operations.

```diff
   # integer left shift with rhs=1
-  mov x11, ruby#1
-  subs x11, x1, x11
+  subs x11, x1, ruby#1
   lsl x12, x11, ruby#1
   asr x13, x12, ruby#1
   cmp x13, x11
-  b.ne #0x106ab60f8
-  mov x11, ruby#1
-  adds x12, x12, x11
+  b.ne #0x10903a0f8
+  adds x12, x12, ruby#1
   mov x1, x12
```

Note that it's fine to cast between i64 and u64 since the bit pattern is
preserved, and the add/sub themselves don't care about the signedness of
the operands.

CMP is just another mnemonic for SUBS.

* YJIT: A64: Split asm.mul() with immediates properly

There is in fact no MUL on A64 that takes an immediate, so this
instruction was using the wrong split method. No current usages of this
form in YJIT.

---------

Co-authored-by: Maxime Chevalier-Boisvert <maxime.chevalierboisvert@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants