[JSC] Add EqualOrUnordered to B3

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Add EqualOrUnordered to B3

[Benjamin Poulain]

Created attachment 267626 [details]
Patch

[Mark Lam]

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.

[Mark Lam]

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.

[Filip Pizlo]

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

[Filip Pizlo]

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

[Filip Pizlo]

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().

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Benjamin Poulain]

(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?

[Filip Pizlo]

(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.

[Benjamin Poulain]

Created attachment 267686 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 267686 [details]
Patch for landing

Clearing flags on attachment: 267686

Committed r194314: <http://trac.webkit.org/changeset/194314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.