Summary: | Add a new Air::Arg kind ZeroReg to let AIR recognise the new instructions/forms accepting zero register in ARM64 | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yijia Huang <yijia_huang> | ||||||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yijia Huang <yijia_huang> | ||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yijia Huang
2021-06-29 15:21:07 PDT
Created attachment 433174 [details]
Patch
Created attachment 433176 [details]
Patch
Created attachment 433181 [details]
Patch
Created attachment 433191 [details]
Patch
Created attachment 433192 [details]
Patch
Created attachment 433195 [details]
Patch
Comment on attachment 433195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433195&action=review > Source/JavaScriptCore/b3/B3LowerToAir.cpp:-1102 > - if (isValidForm(StoreZero32, dest.kind()) && dest.isValidForm(Width32)) Grepping for StoreZero32 and StoreZero64, I still see a lot of them in the code. I think they should get removed and replaced by Store32/64 of Arg::ImmZero everywhere, rather than supporting both until the end of time. > Source/JavaScriptCore/b3/air/AirArg.h:1057 > + case ImmZero: What is the reason for returning false here? Naively I would have expected this to return true since it can be used for integers. Comment on attachment 433195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433195&action=review >> Source/JavaScriptCore/b3/air/AirArg.h:1057 >> + case ImmZero: > > What is the reason for returning false here? Naively I would have expected this to return true since it can be used for integers. Yeah, this should return true I believe > Source/JavaScriptCore/b3/air/opcode_generator.rb:480 > + parseError("Form has an zero register for a non-use argument") nit: I'd use "zero immediate" instead of "zero register" here for consistency. Maybe "Zero immediate must be a use" > Source/JavaScriptCore/b3/air/opcode_generator.rb:483 > + parseError("Form has an zero register for a non-general-purpose argument") ditto, maybe: "Zero immediate must be a general-purpose argument" (In reply to Robin Morisset from comment #8) > Comment on attachment 433195 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433195&action=review > > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:-1102 > > - if (isValidForm(StoreZero32, dest.kind()) && dest.isValidForm(Width32)) > > Grepping for StoreZero32 and StoreZero64, I still see a lot of them in the > code. > I think they should get removed and replaced by Store32/64 of Arg::ImmZero > everywhere, rather than supporting both until the end of time. Thanks for the review. A previous bug report (https://bugs.webkit.org/show_bug.cgi?id=174821) already pointed out the problem that StoreZero is a temporary hack. I intend to remove all StoreZero references in this patch. > > Source/JavaScriptCore/b3/air/AirArg.h:1057 > > + case ImmZero: > > What is the reason for returning false here? Naively I would have expected > this to return true since it can be used for integers. Will fix. (In reply to Saam Barati from comment #9) > Comment on attachment 433195 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433195&action=review > > >> Source/JavaScriptCore/b3/air/AirArg.h:1057 > >> + case ImmZero: > > > > What is the reason for returning false here? Naively I would have expected this to return true since it can be used for integers. > > Yeah, this should return true I believe > > > Source/JavaScriptCore/b3/air/opcode_generator.rb:480 > > + parseError("Form has an zero register for a non-use argument") > > nit: I'd use "zero immediate" instead of "zero register" here for > consistency. > > Maybe "Zero immediate must be a use" > > > Source/JavaScriptCore/b3/air/opcode_generator.rb:483 > > + parseError("Form has an zero register for a non-general-purpose argument") > > ditto, maybe: > "Zero immediate must be a general-purpose argument" Thanks. Will fix. Created attachment 433227 [details]
Patch
Comment on attachment 433227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433227&action=review > Source/JavaScriptCore/ChangeLog:8 > + Add a new operand ImmZero to Air::Arg. So it can be recognized as the zero registers in ARM64 "zero registers" => "zero register" > Source/JavaScriptCore/ChangeLog:9 > + Air opcode. In this case, the new overloads of the instructions accepting zero registers can ditto > Source/JavaScriptCore/ChangeLog:11 > + I'd also add some discussion here about how you're modeling it. e.g, modeling it like a special kind of "Imm", the "ZeroImm" > Source/JavaScriptCore/b3/air/AirArg.h:91 > + // ImmZero is recognized as a zero register for ARM64 > + ImmZero Nit, maybe "ZeroImm" for consistency with the other names, and I also think it reads a bit nicer. > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:681 > +arm64: Store32 U:G:32, D:G:32 > + ImmZero, Addr > + ImmZero, Index > + > +arm64: Store64 U:G:64, D:G:64 > + ImmZero, Addr > + ImmZero, Index Were you going to remove the other instruction? > Source/JavaScriptCore/b3/air/opcode_generator.rb:937 > + when "ImmZero" Why this here? When is this ruby code run? Why do we never do things for IsValidForm for ImmZero? > Source/JavaScriptCore/b3/testb3_2.cpp:1307 > +void testMulNegArgArg(int a, int b) why is this in this patch? Created attachment 433294 [details]
Patch
Created attachment 433295 [details]
Patch
Created attachment 433298 [details]
Patch
Comment on attachment 433298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433298&action=review > Source/JavaScriptCore/ChangeLog:9 > + to different CPUs, the compiler must make instruction set details explicit eventually. remove unicode character at the end > Source/JavaScriptCore/ChangeLog:10 > + Then, Air is introduced, and it has syntax to speak of all of the CPU instructions we I think here I'd just say: "Air is designed to target individual CPU architectures and generate instructions specific to those CPUs" or similar. > Source/JavaScriptCore/ChangeLog:11 > + know about. remove unicode character at the end > Source/JavaScriptCore/ChangeLog:13 > + Previously, B3 and Air don't recognize the ZR register. This problem has been pointed out don't => didn't "ZR register" => "zero register" > Source/JavaScriptCore/ChangeLog:18 > + should be introduced. Its goal is to closely match the CPU instructions accepting a zero immediate "should be introduced" => "is introduced by this patch" > Source/JavaScriptCore/ChangeLog:19 > + or the zero register in X86 and ARM64. Another reason is that with this implementation, the new I don't think we're using the zero immediate for x86, right? I'd say this is for arm64 > Source/JavaScriptCore/ChangeLog:22 > + Here, the ZeroImm is added as a new kind for Air::Arg, which acts as a âhigh levelâ operand more unicode characters here > Source/JavaScriptCore/b3/B3LowerToAir.cpp:1108 > + if (isValidForm(Store32, Arg::ZeroImm, dest.kind()) && dest.isValidForm(Width32)) > + return Inst(Store32, m_value, zeroImm(), dest); This code is already conditionalized under "isARM64" above. I'd just make these instruction variants arm64 only inside AirOpcodes.opcodes > Source/JavaScriptCore/b3/air/AirArg.h:67 > + // ZeroImm is interpreted as a zero register in ARM64 and TrustedImm32(0) in X86 and X86_64 I would not make this a thing on x86. I'd just treat this as modeling how arm64 does things > Source/JavaScriptCore/b3/air/AirArg.h:91 > + remove > Source/JavaScriptCore/b3/air/AirArg.h:760 > + case ZeroImm: this is starting to feel a bit weird to me, and perhaps where the naming is leading us to do weird things. I think I'd just call this the ZeroRegister. > Source/JavaScriptCore/b3/air/AirArg.h:1264 > + case ZeroImm: > + return isValidZeroImmForm(value()); I think this should just be return true, value() doesn't mean anything for ZeroImm. > Source/JavaScriptCore/b3/air/AirArg.h:1369 > +#elif CPU(X86_64) || CPU(X86) > + MacroAssembler::TrustedImm32 asZeroImm() const > + { > + return MacroAssembler::TrustedImm32(0); > + } ditto, I'd just assert at runtime we're arm64, and not have a concept of x86 here. > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:672 > +Store32 U:G:32, ZD:G:32 > + ZeroImm, Addr > + ZeroImm, Index > > -64: StoreZero64 D:G:64 > - Addr > - Index > +64: Store64 U:G:64, D:G:64 > + ZeroImm, Addr > + ZeroImm, Index I'd make these arm64 only Created attachment 433339 [details]
Patch
Comment on attachment 433339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433339&action=review Seems like you introduced a lot of test failures. > Source/JavaScriptCore/b3/air/AirArg.h:760 > + case ZeroReg: should be false > Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:160 > + RELEASE_ASSERT(isValidForm(Store32, Arg::ZeroReg, Arg::Stack)); was this run on x86 before? If so, gotta keep something around for x86. If so, could keep around StoreZero for x86. Comment on attachment 433339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433339&action=review >> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:160 >> + RELEASE_ASSERT(isValidForm(Store32, Arg::ZeroReg, Arg::Stack)); > > was this run on x86 before? If so, gotta keep something around for x86. > > If so, could keep around StoreZero for x86. No. Created attachment 433387 [details]
Patch
Created attachment 433390 [details]
Patch
Created attachment 433416 [details]
Patch
Created attachment 433418 [details]
Patch
Created attachment 433419 [details]
Patch
Created attachment 433420 [details]
Patch
Created attachment 433423 [details]
Patch
Created attachment 433427 [details]
Patch
Comment on attachment 433427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433427&action=review r=me > Source/JavaScriptCore/ChangeLog:13 > + Previously, B3 and Air don't recognize the zero register. This problem has been pointed "B3 and Air" => "Air" > Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:164 > +#elif CPU(X86_64) || CPU(X86) I think you can just have CPU(X86_64) here Created attachment 433444 [details]
Patch
Committed r279889 (239640@main): <https://commits.webkit.org/239640@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433444 [details]. *** Bug 174821 has been marked as a duplicate of this bug. *** |