WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
227509
Add Air opcode add/sub-and-shift for ARM64 and select this instruction in Air
https://bugs.webkit.org/show_bug.cgi?id=227509
Summary
Add Air opcode add/sub-and-shift for ARM64 and select this instruction in Air
Yijia Huang
Reported
2021-06-29 15:18:32 PDT
...
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
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Yijia Huang
Comment 1
2021-07-06 12:55:40 PDT
Created
attachment 432963
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2021-07-06 15:19:16 PDT
<
rdar://problem/80233961
>
Yijia Huang
Comment 3
2021-07-06 15:37:43 PDT
Created
attachment 432983
[details]
Patch
Yijia Huang
Comment 4
2021-07-06 17:27:07 PDT
Created
attachment 432991
[details]
Patch
Yijia Huang
Comment 5
2021-07-06 20:15:09 PDT
Created
attachment 433001
[details]
Patch
Yijia Huang
Comment 6
2021-07-06 23:05:53 PDT
Created
attachment 433015
[details]
Patch
Yijia Huang
Comment 7
2021-07-06 23:13:17 PDT
Created
attachment 433016
[details]
Patch
Filip Pizlo
Comment 8
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.
Filip Pizlo
Comment 9
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.
Saam Barati
Comment 10
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
Yijia Huang
Comment 11
2021-07-07 14:18:18 PDT
Created
attachment 433077
[details]
Patch
Saam Barati
Comment 12
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"?
Yijia Huang
Comment 13
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.
Yijia Huang
Comment 14
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.
Yijia Huang
Comment 15
2021-07-07 21:52:51 PDT
Created
attachment 433118
[details]
Patch
Saam Barati
Comment 16
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.
Yijia Huang
Comment 17
2021-07-08 12:21:29 PDT
Created
attachment 433154
[details]
Patch
Saam Barati
Comment 18
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
Yijia Huang
Comment 19
2021-07-08 13:59:58 PDT
Created
attachment 433161
[details]
Patch
Yijia Huang
Comment 20
2021-07-08 16:32:27 PDT
Created
attachment 433184
[details]
Patch
Yijia Huang
Comment 21
2021-07-08 17:19:21 PDT
Created
attachment 433188
[details]
Patch
EWS
Comment 22
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug