Bug 227509 - Add Air opcode add/sub-and-shift for ARM64 and select this instruction in Air
Summary: Add Air opcode add/sub-and-shift for ARM64 and select this instruction in Air
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yijia Huang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-29 15:18 PDT by Yijia Huang
Modified: 2021-07-08 22:55 PDT (History)
10 users (show)

See Also:


Attachments
Patch (42.78 KB, patch)
2021-07-06 12:55 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (42.80 KB, patch)
2021-07-06 15:37 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (42.80 KB, patch)
2021-07-06 17:27 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (42.88 KB, patch)
2021-07-06 20:15 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (42.80 KB, patch)
2021-07-06 23:05 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (42.80 KB, patch)
2021-07-06 23:13 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (42.00 KB, patch)
2021-07-07 14:18 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (46.25 KB, patch)
2021-07-07 21:52 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (48.71 KB, patch)
2021-07-08 12:21 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (48.07 KB, patch)
2021-07-08 13:59 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (46.16 KB, patch)
2021-07-08 16:32 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (48.79 KB, patch)
2021-07-08 17:19 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2021-06-29 15:18:32 PDT
...
Comment 1 Yijia Huang 2021-07-06 12:55:40 PDT
Created attachment 432963 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-07-06 15:19:16 PDT
<rdar://problem/80233961>
Comment 3 Yijia Huang 2021-07-06 15:37:43 PDT
Created attachment 432983 [details]
Patch
Comment 4 Yijia Huang 2021-07-06 17:27:07 PDT
Created attachment 432991 [details]
Patch
Comment 5 Yijia Huang 2021-07-06 20:15:09 PDT
Created attachment 433001 [details]
Patch
Comment 6 Yijia Huang 2021-07-06 23:05:53 PDT
Created attachment 433015 [details]
Patch
Comment 7 Yijia Huang 2021-07-06 23:13:17 PDT
Created attachment 433016 [details]
Patch
Comment 8 Filip Pizlo 2021-07-07 10:23:03 PDT
Comment on attachment 433016 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433016&action=review

This is really cool!  I don't feel too strongly about it, but I'd recommend not doing canBeInternal/commitInternal for this one.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2616
> +                if (!canBeInternal(right) || !imm(right->child(1)) || right->child(1)->asInt() < 0)

I wouldn't do canBeInternal for this one.  I'd assume that an add-with-shift instruction is no more expensive than an add instruction.  So, if we have a program that does a shift and then uses the result of that shift both directly and for an add, then emitting the add as an add-with-shift instruction is fine.  In fact, it might even be more efficient, since it removes a dependency.  If you have a shift and then an add that depends on the shift, then the add can't run until the shift finishes.  If you have a shift and then an add-with-shift that depends on the same input as the shift, then both the shift and the add-with-shift can execute in parallel.
Comment 9 Filip Pizlo 2021-07-07 10:27:34 PDT
(In reply to Filip Pizlo from comment #8)
> Comment on attachment 433016 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433016&action=review
> 
> This is really cool!  I don't feel too strongly about it, but I'd recommend
> not doing canBeInternal/commitInternal for this one.
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2616
> > +                if (!canBeInternal(right) || !imm(right->child(1)) || right->child(1)->asInt() < 0)
> 
> I wouldn't do canBeInternal for this one.  I'd assume that an add-with-shift
> instruction is no more expensive than an add instruction.  So, if we have a
> program that does a shift and then uses the result of that shift both
> directly and for an add, then emitting the add as an add-with-shift
> instruction is fine.  In fact, it might even be more efficient, since it
> removes a dependency.  If you have a shift and then an add that depends on
> the shift, then the add can't run until the shift finishes.  If you have a
> shift and then an add-with-shift that depends on the same input as the
> shift, then both the shift and the add-with-shift can execute in parallel.

Disregard.  I think you're right to use canBeInternal/commitInternal for this one.
Comment 10 Saam Barati 2021-07-07 12:10:43 PDT
Comment on attachment 433016 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433016&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:556
> +        ASSERT(n != ARM64Registers::sp || m != ARM64Registers::sp);
> +        if (m == ARM64Registers::sp)
> +            std::swap(n, m);

Does this actually work when the shift value isn't zero? Aren't we swapping what gets shifted? Maybe we can assert against this use case instead if my analysis is right?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:564
> +        ASSERT(n != ARM64Registers::sp || m != ARM64Registers::sp);
> +        if (m == ARM64Registers::sp)
> +            std::swap(n, m);

ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:572
> +        if (m == ARM64Registers::sp)
> +            std::swap(n, m);

ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:580
> +        if (m == ARM64Registers::sp)
> +            std::swap(n, m);

ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:588
> +        if (m == ARM64Registers::sp)
> +            std::swap(n, m);

ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:596
> +        if (m == ARM64Registers::sp)
> +            std::swap(n, m);

ditto
Comment 11 Yijia Huang 2021-07-07 14:18:18 PDT
Created attachment 433077 [details]
Patch
Comment 12 Saam Barati 2021-07-07 15:08:25 PDT
Comment on attachment 433077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433077&action=review

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2608
> +                    return Air::Nop;

nit: I'd return Oops to be consistent with tryOpcodeForType

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2614
> +                if (!isValidForm(opcode, Arg::Tmp, Arg::Imm, Arg::Imm, Arg::Tmp)) 

Am I missing something, or is this patch doing anything? Aren't all your instructions you added of the form: "Tmp, Tmp, Imm, Tmp"?
Comment 13 Yijia Huang 2021-07-07 15:45:11 PDT
(In reply to Saam Barati from comment #12)
> Comment on attachment 433077 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433077&action=review
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2608
> > +                    return Air::Nop;
> 
> nit: I'd return Oops to be consistent with tryOpcodeForType

Air::Opcode only has Nop. And JSC::B3::Opcode has Oops.

> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2614
> > +                if (!isValidForm(opcode, Arg::Tmp, Arg::Imm, Arg::Imm, Arg::Tmp)) 
> 
> Am I missing something, or is this patch doing anything? Aren't all your
> instructions you added of the form: "Tmp, Tmp, Imm, Tmp"?

My bad. Will fix it.
Comment 14 Yijia Huang 2021-07-07 15:46:33 PDT
(In reply to Yijia Huang from comment #13)
> (In reply to Saam Barati from comment #12)
> > Comment on attachment 433077 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433077&action=review
> > 
> > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2608
> > > +                    return Air::Nop;
> > 
> > nit: I'd return Oops to be consistent with tryOpcodeForType
> 
> Air::Opcode only has Nop. And JSC::B3::Opcode has Oops.

Found it. Sorry, ignore this.

> 
> > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2614
> > > +                if (!isValidForm(opcode, Arg::Tmp, Arg::Imm, Arg::Imm, Arg::Tmp)) 
> > 
> > Am I missing something, or is this patch doing anything? Aren't all your
> > instructions you added of the form: "Tmp, Tmp, Imm, Tmp"?
> 
> My bad. Will fix it.
Comment 15 Yijia Huang 2021-07-07 21:52:51 PDT
Created attachment 433118 [details]
Patch
Comment 16 Saam Barati 2021-07-08 10:34:50 PDT
Comment on attachment 433118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433118&action=review

Nice. r=me

> Source/JavaScriptCore/b3/testb3_2.cpp:4148
> +            checkUsesInstruction(*code, toCString("add      w0, w0, w1, lsl #", amount).data());

I'd be a bit wary of relying on the register allocator always allocating in these registers. Can't we just use a regexp here instead? I think Keith added a way to do that at some point.

Ditto to all the checks below.
Comment 17 Yijia Huang 2021-07-08 12:21:29 PDT
Created attachment 433154 [details]
Patch
Comment 18 Saam Barati 2021-07-08 13:52:07 PDT
Comment on attachment 433154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433154&action=review

> Tools/Scripts/webkitpy/common/config/contributors.json:5980
> +   "Yijia Huang" : {
> +      "emails" : [
> +         "yijia_huang@apple.com"
> +      ],
> +      "nicks" : [
> +         "yijia"
> +      ],
> +      "status" : "committer"
> +   },

Let's do this in its own patch
Comment 19 Yijia Huang 2021-07-08 13:59:58 PDT
Created attachment 433161 [details]
Patch
Comment 20 Yijia Huang 2021-07-08 16:32:27 PDT
Created attachment 433184 [details]
Patch
Comment 21 Yijia Huang 2021-07-08 17:19:21 PDT
Created attachment 433188 [details]
Patch
Comment 22 EWS 2021-07-08 22:55:35 PDT
Committed r279773 (239542@main): <https://commits.webkit.org/239542@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433188 [details].