Bug 204028 – [WebAssembly] LLIntGenerator should not retain VirtualRegisters used for constants

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 383176 [details]
Patch

[Saam Barati]

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.

[Yusuke Suzuki]

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?

[Saam Barati]

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

[Tadeu Zagallo]

Created attachment 383188 [details]
Patch

[Yusuke Suzuki]

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.

[Tadeu Zagallo]

Created attachment 383202 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383202 [details]
Patch for landing

Clearing flags on attachment: 383202

Committed r252306: <https://trac.webkit.org/changeset/252306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57099536>