Summary: | Add ExtendType to Air::Arg Index to fully utilize address computation in memory instruction for ARM64 | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yijia Huang <yijia_huang> | ||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yijia Huang <yijia_huang> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yijia Huang
2021-07-14 15:53:52 PDT
Created attachment 433545 [details]
Patch
Created attachment 433566 [details]
Patch
Created attachment 433603 [details]
Patch
Created attachment 433614 [details]
Patch
Comment on attachment 433614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433614&action=review > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:130 > + SExt, Is this a thing? > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:131 > + Oops Can this be called None? > Source/JavaScriptCore/b3/B3LowerToAir.cpp:556 > +#if CPU(ARM64) > + if (index->opcode() == ZExt32 && !m_locked.contains(index->child(0))) > + return Arg::index(tmp(base), tmp(index->child(0)), *scale, offset, MacroAssembler::ZExt); > +#endif I feel like the more idiomatic Air way to do this is to hook into isValidIndexForm and teach it about ZExt on arm64. > Source/JavaScriptCore/b3/air/AirArg.h:1491 > int32_t m_scale { 1 }; > Air::Tmp m_base; > Air::Tmp m_index; > + MacroAssembler::Extend m_extend = MacroAssembler::Extend::Oops; I think we only need a single bit for "m_extend" right now. I wonder if we can steal that from elsewhere. One bit for SExt vs nothing. Do we use all 32-bits of m_scale? Comment on attachment 433614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433614&action=review >> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:130 >> + SExt, > > Is this a thing? I think architecturally it is. But do we ever use it? Comment on attachment 433614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433614&action=review >>> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:130 >>> + SExt, >> >> Is this a thing? > > I think architecturally it is. But do we ever use it? maybe it's useful when we do loads of negative properties? I guess we can just add support Created attachment 433640 [details]
Patch
Created attachment 433642 [details]
Patch
Created attachment 433651 [details]
Patch
Created attachment 433655 [details]
Patch
Created attachment 433668 [details]
Patch
Comment on attachment 433614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433614&action=review >> Source/JavaScriptCore/b3/B3LowerToAir.cpp:556 >> +#endif > > I feel like the more idiomatic Air way to do this is to hook into isValidIndexForm and teach it about ZExt on arm64. I agree that it should be hooked to isValidIndexForm. But maybe introducing it to Air opcode is overkill for the extended index at the moment. Please correct me if I am wrong. Created attachment 433688 [details]
Patch
Comment on attachment 433688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433688&action=review > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:128 > + enum Extend { enum class Extend : uint8_t > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:226 > + , extend(extend) > { > } if we're on x86, can we assert that it's None? Maybe in the future, we can change x86's assembler to handle not None type, by having it emit code using the temp register. But for now, we should assert it's not being used. > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1377 > + default: nit: I like to list all cases here, then omit the default case. That way, the compiler will warn if you're missing a case. I think I'd write this then as: "case None" > Source/JavaScriptCore/b3/air/AirArg.h:1491 > + MacroAssembler::Extend m_extend = MacroAssembler::Extend::None; This field should go after m_kind for for better packing size. also "m_extend { MacroAssembler::Extend::None };" is how we write this. Created attachment 433706 [details]
Patch
Comment on attachment 433706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433706&action=review r=me with comments > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:128 > + enum Extend : uint8_t { enum class > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:228 > +#if CPU(X86) || CPU(X86_64) > + ASSERT(extend == None); > +#endif Sorry, I know I said differently earlier. But since you only support this on arm64, I think I'd actually do this as: #if !CPU(ARM64) > Source/JavaScriptCore/b3/B3LowerToAir.cpp:577 > + return indexArg(tmp(base), index->child(0), *scale, offset); can we add a test for add here? Created attachment 433715 [details]
Patch for landing
Created attachment 433716 [details]
Patch for landing
Committed r280013 (239755@main): <https://commits.webkit.org/239755@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433716 [details]. |