Summary: | [WebAssembly] LLIntGenerator should not retain VirtualRegisters used for constants | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||||
Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Tadeu Zagallo
2019-11-08 16:09:21 PST
Created attachment 383176 [details]
Patch
Comment on attachment 383176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383176&action=review > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:272 > + StdUnorderedMap<uint64_t, VirtualRegister> m_constantMap; why StdUnorderedMap? Just because zero might be a key? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:519 > + target->setConstant(); this feels like a lie. I don't get it. Comment on attachment 383176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383176&action=review Commented about RegisterID's m_isConstant flag. > Source/JavaScriptCore/bytecompiler/RegisterID.h:68 > + , m_isConstant(false) Let's put it in field's initializer, like, `bool m_isConstant { false }`. We could also refactor `m_refCount` and `m_isTemporary` fields. And this constructor can delegate to `RegisterID(VirtualRegister virtualRegister)` like, `: RegisterID(VirtualRegister(index))`. > Source/JavaScriptCore/bytecompiler/RegisterID.h:136 > + bool m_isConstant; It is a bit scary to me. I assume RegisterID is immutable one so long as it is live. So it looks strange to me when `isConstant()` starts returning different value in different code point for the exact same RegisterID. Can we track this status differently instead of having it in RegisterID? Comment on attachment 383176 [details]
Patch
discussed offline with Tadeu. This isn't sound, since a local can be written to with a non constant. It's not a static property of a local
Created attachment 383188 [details]
Patch
Comment on attachment 383188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383188&action=review r=me with nit. > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:510 > + if (auto it = m_constantMap.find(value); it != m_constantMap.end()) > + source = it->second; > + else { > + source = VirtualRegister(FirstConstantRegisterIndex + m_codeBlock->m_constants.size()); > + m_codeBlock->m_constants.append(value); > + m_constantMap.emplace(value, source); > + } We are looking up dictionary twice and it is unnecessary, (1) one for m_constantMap.find, and (2) one for m_constantMap.emplace. You can use the return-value of std::unrodered_map::emplace to avoid looking up unordered_map twice here, since emplace does nothing if entry already exists. Created attachment 383202 [details]
Patch for landing
Comment on attachment 383202 [details] Patch for landing Clearing flags on attachment: 383202 Committed r252306: <https://trac.webkit.org/changeset/252306> All reviewed patches have been landed. Closing bug. |