Bug 191802 – [WebAssembly] Change BBQ to generate Air IR

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

[Yusuke Suzuki]

Template JIT like DFG.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #0)
> Template JIT like DFG.

I was thinking more like the baseline JIT. It might also be worth considering:
- an interpreter
- lazy compilation

[Yusuke Suzuki]

(In reply to Saam Barati from comment #1)
> (In reply to Yusuke Suzuki from comment #0)
> > Template JIT like DFG.
> 
> I was thinking more like the baseline JIT. It might also be worth
> considering:

Right, the implementation should be like baseline JIT.

> - an interpreter

If we need an interpreter tier, anyway, I think bytecode for wasm is additionally required. After looking into the wasm format, wasm interpreter should target its own bytecode instead of running wasm code directly, because wasm interpreter needs to decode LEB everytime we execute wasm ops, which is super tricky and costly.

> - lazy compilation

[Yusuke Suzuki]

I think the baseline JIT for wasm can be do some optimizations done in DFG.
One of the good feature of stack VM is that the lifetimes of temporary values are roughly represented without any analysis. This information allows us to stay / spill values in the machine registers, which removes store/load operations :)

[Yusuke Suzuki]

Created attachment 356297 [details]
WIP

[Saam Barati]

(In reply to Yusuke Suzuki from comment #4)
> Created attachment 356297 [details]
> WIP

👌🏼

[Saam Barati]

(In reply to Yusuke Suzuki from comment #3)
> I think the baseline JIT for wasm can be do some optimizations done in DFG.
> One of the good feature of stack VM is that the lifetimes of temporary
> values are roughly represented without any analysis. This information allows
> us to stay / spill values in the machine registers, which removes store/load
> operations :)

This makes sense. It should be mostly free to have some basic register allocation.

I wonder if it’s also worth having some kind of basic instruction selection.

[Yusuke Suzuki]

Created attachment 357108 [details]
WIP

[EWS Watchlist]

Attachment 357108 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/RegisterBank.h:47:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:49:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:51:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:71:  NUM_REGS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:369:  gpr_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:370:  fpr_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmRailgunJIT.cpp:312:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 7 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 357110 [details]
WIP

[EWS Watchlist]

Attachment 357110 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/RegisterBank.h:47:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:49:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:51:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:71:  NUM_REGS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:369:  gpr_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:370:  fpr_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmRailgunJIT.cpp:321:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmRailgunJIT.cpp:344:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 8 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

I'm going to take this over. I think if we do this right we can get between 4-8% faster on JetStream 2.

[Yusuke Suzuki]

Created attachment 360063 [details]
Patch

I've just cleaned up the uploaded patch before passing this to Saam.

[EWS Watchlist]

Attachment 360063 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:282:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:288:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:289:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:321:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:344:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:523:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:543:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmTemplateJIT.cpp:548:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:47:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:49:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:51:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:71:  NUM_REGS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:369:  gpr_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/jit/RegisterBank.h:370:  fpr_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:216:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:222:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:223:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1018:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1086:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1185:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmValidate.cpp:130:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmValidate.cpp:140:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmValidate.cpp:141:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmValidate.cpp:282:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmValidate.cpp:328:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmValidate.cpp:340:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:336:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:365:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:412:  Use 'WTF::Optional<>' instead of 'std::optional<>'.  [runtime/wtf_optional] [4]
Total errors found: 29 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

I spoke with Yusuke and Keith. Our plan is to just generate Air directly, and write a trivial register allocator over Air.

[Saam Barati]

Created attachment 360163 [details]
WIP

[Saam Barati]

Created attachment 360171 [details]
WIP

[Saam Barati]

Created attachment 360204 [details]
WIP

I don't think I'm too far off from testing out some simple programs.

[Saam Barati]

Created attachment 360205 [details]
WIP

[Saam Barati]

Comment on attachment 360205 [details]
WIP

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2771
> +    return addShift(Lshift32, arg0, arg1, result);

Oops

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2815
> +    append(NegateDouble, arg0, result);

Oops

[Saam Barati]

Created attachment 360309 [details]
WIP

[Saam Barati]

Created attachment 360310 [details]
WIP

[Saam Barati]

Comment on attachment 360310 [details]
WIP

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:298
> +        m_currentBlock->setSuccessors(continuation, failed);

Oops

[Saam Barati]

Created attachment 360328 [details]
WIP

[Saam Barati]

Created attachment 360374 [details]
WIP

[Saam Barati]

Created attachment 360382 [details]
WIP

Seems close to compiling.

[Saam Barati]

Created attachment 360400 [details]
WIP

It compiles. Now to see how wrong it is...

[Saam Barati]

Created attachment 360415 [details]
WIP

Some tests run... Some don't

[Saam Barati]

Created attachment 360420 [details]
WIP

[Saam Barati]

Created attachment 360421 [details]
WIP

Passing like 70% of tests.

[Saam Barati]

Created attachment 360423 [details]
WIP

[Saam Barati]

Comment on attachment 360423 [details]
WIP

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3031
> +    append(FloorDouble, arg0, result);

Oops. Add test

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3110
> +        append(SubDouble, result, arg0);

Oops

[Saam Barati]

Created attachment 360444 [details]
WIP

[Saam Barati]

Created attachment 360452 [details]
WIP

Almost all tests pass. Just a few spec-tests left.

[Saam Barati]

Created attachment 360495 [details]
WIP

All wasm tests pass

[Saam Barati]

Created attachment 360498 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/47651718>

[Saam Barati]

Created attachment 360531 [details]
WIP

[Saam Barati]

Created attachment 360648 [details]
patch

[EWS Watchlist]

Attachment 360648 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1776:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2061:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2197:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2219:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3029:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3383:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 360652 [details]
patch

[Saam Barati]

Comment on attachment 360652 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:593
> +    Vector<Tmp, 8> m_freeFPTemps;
> +    Vector<Tmp, 8> m_freeGPTemps;

These should be removed.

[EWS Watchlist]

Attachment 360652 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1776:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2061:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2197:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2219:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3029:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3383:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 360655 [details]
patch

[EWS Watchlist]

Attachment 360655 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1757:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2042:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2178:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2200:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3010:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3364:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 360655 [details]
patch

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

r=me.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:305
> +        // FIXME: Fins a way to use origin here.

typo: Fins => Find

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1290
> +    auto storeArg = [&] () {
> +        if (Arg::isValidAddrForm(offset, B3::widthForBytes(sizeOfStoreOp(op))))
> +            return Arg::addr(pointer, offset);
> +        immTmp = g64();
> +        newPtr = g64();
> +        append(Move, Arg::bigImm(offset), immTmp);
> +        append(Add64, immTmp, pointer, newPtr);
> +        return Arg::addr(newPtr);
> +    };

Why not just make an Arg::Addr and pass that in below rather than have a lambda that you call every time?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2463
> +        // the numbers are would be positive anyway as a signed integer. Since we cannot materialize constants into fprs we have b3 do it

I think you mean air :P

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2541
> +        // the numbers would be positive anyway as a signed integer. Since we cannot materialize constants into fprs we have b3 do it

ditto.

[Saam Barati]

Comment on attachment 360655 [details]
patch

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

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:305
>> +        // FIXME: Fins a way to use origin here.
> 
> typo: Fins => Find

fixed

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1290
>> +    };
> 
> Why not just make an Arg::Addr and pass that in below rather than have a lambda that you call every time?

sounds good. Will do the same for load.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2463
>> +        // the numbers are would be positive anyway as a signed integer. Since we cannot materialize constants into fprs we have b3 do it
> 
> I think you mean air :P

Gonna remove these comments.

[Saam Barati]

Created attachment 360669 [details]
patch for landing

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/240752/webkit