Bug 204028

Summary: [WebAssembly] LLIntGenerator should not retain VirtualRegisters used for constants
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
Patch for landing none

Description Tadeu Zagallo 2019-11-08 16:09:21 PST
...
Comment 1 Tadeu Zagallo 2019-11-08 16:24:03 PST
Created attachment 383176 [details]
Patch
Comment 2 Saam Barati 2019-11-08 16:30:30 PST
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 3 Yusuke Suzuki 2019-11-08 16:40:35 PST
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 4 Saam Barati 2019-11-08 16:42:25 PST
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
Comment 5 Tadeu Zagallo 2019-11-08 17:48:40 PST
Created attachment 383188 [details]
Patch
Comment 6 Yusuke Suzuki 2019-11-08 17:57:41 PST
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.
Comment 7 Tadeu Zagallo 2019-11-08 22:20:55 PST
Created attachment 383202 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2019-11-08 23:05:23 PST
Comment on attachment 383202 [details]
Patch for landing

Clearing flags on attachment: 383202

Committed r252306: <https://trac.webkit.org/changeset/252306>
Comment 9 WebKit Commit Bot 2019-11-08 23:05:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-11-11 17:04:22 PST
<rdar://problem/57099536>