Bug 161569 - Add support for WASM Loops and Branches
Summary: Add support for WASM Loops and Branches
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks: 146064
  Show dependency treegraph
 
Reported: 2016-09-03 12:12 PDT by Keith Miller
Modified: 2016-09-07 10:14 PDT (History)
8 users (show)

See Also:


Attachments
WIP (56.52 KB, patch)
2016-09-03 12:13 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (66.97 KB, patch)
2016-09-06 10:23 PDT, Keith Miller
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-09-03 12:12:50 PDT
Add support for WASM Loops and Branches
Comment 1 Keith Miller 2016-09-03 12:13:07 PDT
Created attachment 287866 [details]
WIP
Comment 2 Keith Miller 2016-09-06 10:23:18 PDT
Created attachment 288032 [details]
Patch
Comment 3 Keith Miller 2016-09-06 14:58:51 PDT
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.
Comment 4 Benjamin Poulain 2016-09-06 18:52:45 PDT
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)
)
Comment 5 Csaba Osztrogonác_OOO_until_21st_Aug 2016-09-07 02:08:22 PDT
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.
Comment 6 Csaba Osztrogonác_OOO_until_21st_Aug 2016-09-07 02:33:11 PDT
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
Comment 7 Keith Miller 2016-09-07 09:25:53 PDT
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.
Comment 8 Keith Miller 2016-09-07 10:01:28 PDT
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.
Comment 9 Keith Miller 2016-09-07 10:14:15 PDT
Committed r205552: <http://trac.webkit.org/changeset/205552>