Bug 151974 – [JSC] Add Float support to B3

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

[Benjamin Poulain]

[JSC] Add Float support to B3

[Benjamin Poulain]

Created attachment 266835 [details]
Patch

[Benjamin Poulain]

*** Bug 151921 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

*** Bug 151903 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:5937:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6340:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6341:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6342:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6343:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6345:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6377:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6378:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6379:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6380:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6381:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6382:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 13 in 38 files


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

[Benjamin Poulain]

Created attachment 266836 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:5937:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6340:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6341:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6342:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6343:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6345:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6377:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6378:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6379:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6380:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6381:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6382:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 13 in 38 files


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

[Benjamin Poulain]

Created attachment 266837 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:5937:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6340:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6341:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6342:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6343:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6344:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6345:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6377:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6378:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6379:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6380:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6381:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6382:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 13 in 38 files


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

[Filip Pizlo]

Comment on attachment 266837 [details]
Patch

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

I think you're just missing moveFloatConditionally, which corresponds to Float = Select(Int, Float, Float).  But I guess that depends on whether or not you believe that movsd can efficiently be used instead of movss.

> Source/JavaScriptCore/ChangeLog:8
> +        This patch adds basic float support to B3.

Basic or comprehensive?

> Source/JavaScriptCore/assembler/MacroAssembler.h:1365
> +
> +    void moveDoubleConditionallyFloat(DoubleCondition cond, FPRegisterID left, FPRegisterID right, FPRegisterID src, FPRegisterID dest)
> +    {
> +        Jump falseCase = branchFloat(invert(cond), left, right);
> +        moveDouble(src, dest);
> +        falseCase.link(this);
> +    }

What about moveFloatConditionally?

> Source/JavaScriptCore/b3/B3Generate.cpp:78
> +        reduceDoubleToFloat(procedure);
> +

Any particular reason why this isn't run after reduceStrength()?

Actually, why isn't this just part of reduceStrength()?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:783
> +        case Float:

This doesn't seem right.  Is it really optimal to use movsd when we just need movss?

[Filip Pizlo]

Comment on attachment 266837 [details]
Patch

We talked about the moveFloat/moveFloatConditionally issue offline.

[Filip Pizlo]

Comment on attachment 266837 [details]
Patch

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

> Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:85
> +    HashSet<Value*> candidates;

Why not IndexSet<Value> candidates?

[Benjamin Poulain]

Committed r193683: <http://trac.webkit.org/changeset/193683>

[Geoffrey Garen]

Comment on attachment 266837 [details]
Patch

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

> Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:72
> +// Such operation arise in JS because there are is no Float type

are is

> Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:76
> +// Using float in place remove the useless conversion, and the float

removes

> Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:77
> +// ops is sometime massively cheaper (SQRT for example).

op