Bug 161569 – Add support for WASM Loops and Branches

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add support for WASM Loops and Branches

[Keith Miller]

Created attachment 287866 [details]
WIP

[Keith Miller]

Created attachment 288032 [details]
Patch

[Keith Miller]

Comment on attachment 288032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review

> Source/JavaScriptCore/wasm/WASMFunctionParser.h:173
> +        case OpType::Loop: {
> +            if (!m_context.addLoop())
> +                return false;
> +            return parseBlock();
> +        }

I'll fix this indentation before landing.

[Benjamin Poulain]

Comment on attachment 288032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review

> Source/JavaScriptCore/ChangeLog:19
> +        Another interesting thing of note is that we now only allocate the
> +        continuation basic block. This is beneficial when the continuation

we now only allocate the continuation basic block *lazily*?

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:48
> +static const bool verbose = false;

In B3, we started using anonymous namespaces like LLVM. This would be in the anonymous namespace instead of being a static variable.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:74
> +        LazyBlock(BasicBlock* block)

You could have 
    LazyBlock() = default;
to simplify ControlData.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:79
> +        explicit operator bool() { return m_block; }

!!m_block

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:97
> +        BasicBlock* m_block;

Can be 
    BasicBlock* m_block { nullptr; }

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:102
> +        ControlData(Optional<Vector<Variable*>>&& _stack, BasicBlock* _loopTarget = nullptr)

What's with the "_" prefix of the variables? 

Shouldn't there be a ControlData() = default; too for when you start one of those?

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:103
> +            : continuation(nullptr)

This could go away if you do the changes above.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:125
> +        bool isLoop() { return loopTarget; }

const + !!loopTarget

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:289
> +        return true;

Since you are not changing m_currentBlock, wouldn't you add instruction past the terminal?

Say:
(block
  (block
      (foo)
      (return)
  )
  (bar)
)

[Csaba Osztrogonác]

Comment on attachment 288032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review

> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:1
> +/*

Please add this new file to the cmake buildsystem too - Source/JavaScriptCore/CMakeLists.txt
(Don't need to remove B3CallingConventions.cpp, because it wasn't added ever.)

> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:31
> +#include <wtf/NeverDestroyed.h>
> +
> +#if ENABLE(WEBASSEMBLY)

Please move this include after the #if guard.

[Csaba Osztrogonác]

Comment on attachment 288032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review

> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:35
> +namespace JSC {
> +
> +namespace WASM {

Use namespace JSC { namespace WASM { style similar to all JSC sources.
It would be great if you could fix this style in other WASM sources too.

> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:50
> +} // namespace B3
> +
> +} // namespace JSC

} } // namespace JSC::WASM

> Source/JavaScriptCore/wasm/WASMCallingConvention.h:40
> +namespace JSC {
> +
> +namespace WASM {

ditto

> Source/JavaScriptCore/wasm/WASMCallingConvention.h:92
> +} // namespace WASM
> +
> +} // namespace JSC

ditto

[Keith Miller]

Comment on attachment 288032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:48
>> +static const bool verbose = false;
> 
> In B3, we started using anonymous namespaces like LLVM. This would be in the anonymous namespace instead of being a static variable.

Changed.

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:102
>> +        ControlData(Optional<Vector<Variable*>>&& _stack, BasicBlock* _loopTarget = nullptr)
> 
> What's with the "_" prefix of the variables? 
> 
> Shouldn't there be a ControlData() = default; too for when you start one of those?

Whoops deleted. 

Right now we don't need one. Although when I add if I'll need to add one then.

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:289
>> +        return true;
> 
> Since you are not changing m_currentBlock, wouldn't you add instruction past the terminal?
> 
> Say:
> (block
>   (block
>       (foo)
>       (return)
>   )
>   (bar)
> )

Whoops I forgot to add the code that prevents adding instructions when we are in unreachable.

The change is to add a check at the top of parseExpression that just returns if we are in unreachable code. Unless the value is a control value.

[Keith Miller]

Comment on attachment 288032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review

>> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:31
>> +#if ENABLE(WEBASSEMBLY)
> 
> Please move this include after the #if guard.

Fixed.

>> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:35
>> +namespace WASM {
> 
> Use namespace JSC { namespace WASM { style similar to all JSC sources.
> It would be great if you could fix this style in other WASM sources too.

done.

[Keith Miller]

Committed r205552: <http://trac.webkit.org/changeset/205552>