Bug 152425 - [JSC] Add EqualOrUnordered to B3
Summary: [JSC] Add EqualOrUnordered to B3
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: Benjamin Poulain
URL:
Keywords:
: 152358 (view as bug list)
Depends on:
Blocks: 150279
  Show dependency treegraph
 
Reported: 2015-12-18 06:58 PST by Benjamin Poulain
Modified: 2015-12-19 08:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (27.13 KB, patch)
2015-12-18 06:59 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (27.28 KB, patch)
2015-12-19 07:53 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-12-18 06:58:20 PST
[JSC] Add EqualOrUnordered to B3
Comment 1 Benjamin Poulain 2015-12-18 06:59:24 PST
Created attachment 267626 [details]
Patch
Comment 2 Mark Lam 2015-12-18 09:33:39 PST
Comment on attachment 267626 [details]
Patch

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

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1158
> +                return createRelCond(MacroAssembler::BelowOrEqual, MacroAssembler::DoubleEqualOrUnordered);

Why is the first condition "MacroAssembler::BelowOrEqual" instead of "MacroAssembler::Equal"?  Or is this a bogus condition because this condition only applies to doubles?  If so, can we have a comment here similar to the one in "case Above:" below.  Ideally, an assertion that (value->child(0)->type() == Float) would be good if this path is not to be taken for ints.  Adding an analogous assertion (ensuring children are Int typed) for Above, Below, AboveEqual, BelowEqual would be good too (but can be done in a separate patch).

LGTM otherwise.
Comment 3 Mark Lam 2015-12-18 09:46:04 PST
Comment on attachment 267626 [details]
Patch

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

>> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1158
>> +                return createRelCond(MacroAssembler::BelowOrEqual, MacroAssembler::DoubleEqualOrUnordered);
> 
> Why is the first condition "MacroAssembler::BelowOrEqual" instead of "MacroAssembler::Equal"?  Or is this a bogus condition because this condition only applies to doubles?  If so, can we have a comment here similar to the one in "case Above:" below.  Ideally, an assertion that (value->child(0)->type() == Float) would be good if this path is not to be taken for ints.  Adding an analogous assertion (ensuring children are Int typed) for Above, Below, AboveEqual, BelowEqual would be good too (but can be done in a separate patch).
> 
> LGTM otherwise.

Spoke with Ben offline. r=me with fixes.
Comment 4 Filip Pizlo 2015-12-18 09:52:48 PST
Comment on attachment 267626 [details]
Patch

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

Please change the name of the constant folding method.

> Source/JavaScriptCore/b3/B3ConstDoubleValue.cpp:161
> +TriState ConstDoubleValue::equalOrUnordered(const Value* other) const

This should be called equalOrUnorderedConstant
Comment 5 Filip Pizlo 2015-12-18 10:51:34 PST
*** Bug 152358 has been marked as a duplicate of this bug. ***
Comment 6 Filip Pizlo 2015-12-18 10:56:55 PST
Comment on attachment 267626 [details]
Patch

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

Revoking r+ because there are a couple of things that need to be changed before this lands.

> Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:109
> +            case EqualOrUnordered:

You also need a case for EqualOrUnordered in the main switch statement in B3::LowerToAir::lower().
Comment 7 Filip Pizlo 2015-12-18 18:32:05 PST
(In reply to comment #6)
> Comment on attachment 267626 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267626&action=review
> 
> Revoking r+ because there are a couple of things that need to be changed
> before this lands.
> 
> > Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp:109
> > +            case EqualOrUnordered:
> 
> You also need a case for EqualOrUnordered in the main switch statement in
> B3::LowerToAir::lower().

Hmmm, I guess you don't have to do this in this patch since I guess it triggers https://bugs.webkit.org/show_bug.cgi?id=150903.  Up to you.
Comment 8 Filip Pizlo 2015-12-18 18:32:48 PST
Comment on attachment 267626 [details]
Patch

Flipping back to r+ but please rename Value::equalOrUnordered to Value::equalOrUnorderedConstant to match the other constant-folding method names.
Comment 9 Benjamin Poulain 2015-12-19 00:14:52 PST
(In reply to comment #8)
> Comment on attachment 267626 [details]
> Patch
> 
> Flipping back to r+ but please rename Value::equalOrUnordered to
> Value::equalOrUnorderedConstant to match the other constant-folding method
> names.

Value::equalOrUnordered() also handle single unordered arg, that's why it does not have the "Constant" suffix.

Do you still require that renaming?
Comment 10 Filip Pizlo 2015-12-19 06:52:13 PST
(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 267626 [details]
> > Patch
> > 
> > Flipping back to r+ but please rename Value::equalOrUnordered to
> > Value::equalOrUnorderedConstant to match the other constant-folding method
> > names.
> 
> Value::equalOrUnordered() also handle single unordered arg, that's why it
> does not have the "Constant" suffix.
> 
> Do you still require that renaming?

Good point. But the normal Equal method could always return false if 'this' is nan no matter whether the other argument is a constant. 

So I think we use the "Constant" in the method name to signify that this is a constant folding helper. Better to stick with that.
Comment 11 Benjamin Poulain 2015-12-19 07:53:42 PST
Created attachment 267686 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2015-12-19 08:52:38 PST
Comment on attachment 267686 [details]
Patch for landing

Clearing flags on attachment: 267686

Committed r194314: <http://trac.webkit.org/changeset/194314>
Comment 13 WebKit Commit Bot 2015-12-19 08:52:42 PST
All reviewed patches have been landed.  Closing bug.