...
Created attachment 432963 [details] Patch
<rdar://problem/80233961>
Created attachment 432983 [details] Patch
Created attachment 432991 [details] Patch
Created attachment 433001 [details] Patch
Created attachment 433015 [details] Patch
Created attachment 433016 [details] Patch
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.
(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 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
Created attachment 433077 [details] Patch
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"?
(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.
(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.
Created attachment 433118 [details] Patch
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.
Created attachment 433154 [details] Patch
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
Created attachment 433161 [details] Patch
Created attachment 433184 [details] Patch
Created attachment 433188 [details] Patch
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].