Bug 152425

Created attachment 267626 [details] Patch

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

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

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

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

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

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

Created attachment 267686 [details] Patch for landing

Comment on attachment 267686 [details] Patch for landing Clearing flags on attachment: 267686 Committed r194314: <http://trac.webkit.org/changeset/194314>

All reviewed patches have been landed. Closing bug.