Bug 151322 – [JSC] Support Doubles with B3's Sub

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

[Mark Lam]

[JSC] Support Doubles with B3's Sub.

[Mark Lam]

Created attachment 265613 [details]
proposed patch.

[Mark Lam]

Created attachment 265619 [details]
patch 2: added -M_PI and -1 to the test operands.

[Geoffrey Garen]

Comment on attachment 265619 [details]
patch 2: added -M_PI and -1 to the test operands.

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

r=me

> Source/JavaScriptCore/b3/testb3.cpp:4003
> +// Some convenience functions for breviy.
> +double posInfinity()
> +{
> +    return std::numeric_limits<double>::infinity();
> +}
> +
> +double negInfinity()
> +{
> +    return -std::numeric_limits<double>::infinity();
> +}

These functions plus their explanatory comment are net less brevity, since they are called in only one place.

> Source/JavaScriptCore/b3/testb3.cpp:4027
> +#define DOUBLE_OPERAND(x__) { #x__, x__ }
> +
> +    static const std::array<DoubleOperand, 9> operands = {{
> +        DOUBLE_OPERAND(M_PI),
> +        DOUBLE_OPERAND(-M_PI),
> +        DOUBLE_OPERAND(1),
> +        DOUBLE_OPERAND(-1),
> +        DOUBLE_OPERAND(0),
> +        DOUBLE_OPERAND(negativeZero()),
> +        DOUBLE_OPERAND(posInfinity()),
> +        DOUBLE_OPERAND(negInfinity()),
> +        DOUBLE_OPERAND(PNaN)
> +    }};
> +
> +#undef DOUBLE_OPERAND

This code would be more readable and fewer characters if you just wrote out '{ "1", 1 }', etc.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:4139:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:4153:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:4154:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 4 files


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

[Benjamin Poulain]

Comment on attachment 265619 [details]
patch 2: added -M_PI and -1 to the test operands.

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

+1

Good idea on the testing.

> Source/JavaScriptCore/b3/testb3.cpp:4063
> +            std::string testStr =                           \
> +                std::string(#test) + "(" + a.name + ", " + b.name + ")"; \
> +            if (!shouldRun(testStr.c_str()))                \

WTFString instead of std::string?

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:174
> +SubDouble U:F, UD:F

Lets put this with the Integer Substract opcodes.

[Mark Lam]

Thanks for the reviews.  Responses below.

(In reply to comment #3)
> > Source/JavaScriptCore/b3/testb3.cpp:4003
> > +// Some convenience functions for breviy.
> > +double posInfinity()
> > +{
> > +    return std::numeric_limits<double>::infinity();
> > +}
> > +
> > +double negInfinity()
> > +{
> > +    return -std::numeric_limits<double>::infinity();
> > +}
> 
> These functions plus their explanatory comment are net less brevity, since
> they are called in only one place.

The brevity is in the test output.  For example:

testSubArgImmDouble(posInfinity(), posInfinity())...
testSubArgImmDouble(posInfinity(), posInfinity()): OK!
testSubArgImmDouble(posInfinity(), negInfinity())...
testSubArgImmDouble(posInfinity(), negInfinity()): OK!

vs ...

testSubArgImmDouble(std::numeric_limits<double>::infinity(), std::numeric_limits<double>::infinity())...
testSubArgImmDouble(std::numeric_limits<double>::infinity(), std::numeric_limits<double>::infinity()): OK!
testSubArgImmDouble(std::numeric_limits<double>::infinity(), -std::numeric_limits<double>::infinity())...
testSubArgImmDouble(std::numeric_limits<double>::infinity(), -std::numeric_limits<double>::infinity()): OK!

I find the first form easier to read.  I've updated the comment to indicate that this is for brevity of the test output.  Do you still object to using these wrapper functions?

> > Source/JavaScriptCore/b3/testb3.cpp:4027
> > +#define DOUBLE_OPERAND(x__) { #x__, x__ }
> > +
> > +    static const std::array<DoubleOperand, 9> operands = {{
> > +        DOUBLE_OPERAND(M_PI),
> > +        DOUBLE_OPERAND(-M_PI),
> > +        DOUBLE_OPERAND(1),
> > +        DOUBLE_OPERAND(-1),
> > +        DOUBLE_OPERAND(0),
> > +        DOUBLE_OPERAND(negativeZero()),
> > +        DOUBLE_OPERAND(posInfinity()),
> > +        DOUBLE_OPERAND(negInfinity()),
> > +        DOUBLE_OPERAND(PNaN)
> > +    }};
> > +
> > +#undef DOUBLE_OPERAND
> 
> This code would be more readable and fewer characters if you just wrote out
> '{ "1", 1 }', etc.

I still think it's better to use the macro.  Using the macro means less copy and pasting for each of the values (once as a string, once as a value).  Granted, the list is not large, and hence, the cut-and-pasting is not that big an issue.  In terms of readability, if you think of it as a pseudo constructor of DoubleOperand structs, it actually reads quite clearly.

Do you strongly object to using the DOUBLE_OPERAND macro?

(In reply to comment #5)
> > Source/JavaScriptCore/b3/testb3.cpp:4063
> > +            std::string testStr =                           \
> > +                std::string(#test) + "(" + a.name + ", " + b.name + ")"; \
> > +            if (!shouldRun(testStr.c_str()))                \
> 
> WTFString instead of std::string?

Fixed.

> > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:174
> > +SubDouble U:F, UD:F
> 
> Lets put this with the Integer Substract opcodes.

OK.  I searched for AddDouble and inserted it in alphabetical order thereafter.  Turns out, AddDouble is not group with the rest of the Adds.  I'll fix the SubDouble case (as you suggested) in this patch.  I'll fix the Add case later when I refactor the Add tests to use the new test macros.

[Mark Lam]

Created attachment 265667 [details]
patch 3: addressed with feedback.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:4287:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:4301:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:4302:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 4 files


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

[Csaba Osztrogonác]

Comment on attachment 265619 [details]
patch 2: added -M_PI and -1 to the test operands.

Cleared Geoffrey Garen's review+ from obsolete attachment 265619 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Filip Pizlo]

Comment on attachment 265667 [details]
patch 3: addressed with feedback.

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

LGTM other than minor nits.

> Source/JavaScriptCore/b3/testb3.cpp:4288
> +        String testStr = String(#test) + "(" + a.name + ")"; \

You could also do:

CString testStr = toCString(#test, "(", a.name, ")");

It's not clear that this is better, but it means you can avoid the ascii conversion, below.

> Source/JavaScriptCore/b3/testb3.cpp:4293
> +                const char* testName = testStr.ascii().data(); \

... then you'd just do testStr.data() here.

> Source/JavaScriptCore/b3/testb3.cpp:4294
> +                dataLog(testName, "...\n");             \

Actually, you could just pass testStr here directly.  Both String and CString can be passed directly to dataLog().

> Source/JavaScriptCore/b3/testb3.cpp:4296
> +                dataLog(testName, ": OK!\n");           \

Ditto.

> Source/JavaScriptCore/b3/testb3.cpp:4303
> +            String testStr = String(#test) + "(" + a.name + ", " + b.name + ")"; \

See above.

> Source/JavaScriptCore/b3/testb3.cpp:4312
> +            }));                                            \

This line seems oddly indented - maybe it needs four more spaces?

[Mark Lam]

For the record, Geoff agreed with keeping the negInfinity() and posInfinity() functions.  Geoff thinks de-macrofying the DoubleOperand initializing values is better, and I've applied this change. I've also applied Fil's suggestion to use CString instead, and also fixed the indentation issue.

Landed in r192519: <http://trac.webkit.org/r192519>.