Bug 160954 – [JSC] ArithSqrt should work with any argument type

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

[Benjamin Poulain]

[JSC] ArithSqrt should work with any argument type

[Benjamin Poulain]

Created attachment 286364 [details]
Patch

[Saam Barati]

Comment on attachment 286364 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:24
> +        with Filip, we decided not to use ToNumber+ArithSqrt despite

Out of curiosity, why not go with this design?
We have variants of Untyped math support in different places.
I wonder if we'd be better of just inserting ToNumber everywhere
instead of explicitly supporting UntypedUse everywhere. What do you think?

> JSTests/stress/arith-sqrt-on-various-types.js:117
> +    if (numberOfDFGCompiles(opaqueSqrtForCSE) > 1)

Is this compatible with OSR exit fuzzing?

[Benjamin Poulain]

(In reply to comment #2)
> Out of curiosity, why not go with this design?
> We have variants of Untyped math support in different places.
> I wonder if we'd be better of just inserting ToNumber everywhere
> instead of explicitly supporting UntypedUse everywhere. What do you think?

Filip does not like the idea because we do not have explicit ways to relate those operations.

If you were to insert an operation between the "ToNumber" and "ArithSqrt", and that operation exits, there is no way to execute the Math.sqrt without calling ToNumber again.
There is also the risk of changing ToNumber such that it exits in some cases.

To split an op into multiple nodes, the side effects should be done on the last node.

> > JSTests/stress/arith-sqrt-on-various-types.js:117
> > +    if (numberOfDFGCompiles(opaqueSqrtForCSE) > 1)
> 
> Is this compatible with OSR exit fuzzing?

Hum, it is not! I did not think about that.

Any idea how to test this properly? I want to enforce that we no longer OSR Exit like crazy.
Maybe I should just have a function to tell this particular block should not be subject to fuzzing.

[Saam Barati]

(In reply to comment #3)
> (In reply to comment #2)
> > Out of curiosity, why not go with this design?
> > We have variants of Untyped math support in different places.
> > I wonder if we'd be better of just inserting ToNumber everywhere
> > instead of explicitly supporting UntypedUse everywhere. What do you think?
> 
> Filip does not like the idea because we do not have explicit ways to relate
> those operations.
> 
> If you were to insert an operation between the "ToNumber" and "ArithSqrt",
> and that operation exits, there is no way to execute the Math.sqrt without
> calling ToNumber again.
> There is also the risk of changing ToNumber such that it exits in some cases.
I think the concern of doing something that exits between the two nodes
is a real concern. However, if ToNumber decides to start exiting at some
point for some reason, I think that'd be fine. ToNumber can't exit to
to_number after it has already performed its side effects. So I think
that this problem exists irrespective of this patch. Regardless, I agree
that we should not split this into two nodes.

> 
> To split an op into multiple nodes, the side effects should be done on the
> last node.
Right.
> 
> > > JSTests/stress/arith-sqrt-on-various-types.js:117
> > > +    if (numberOfDFGCompiles(opaqueSqrtForCSE) > 1)
> > 
> > Is this compatible with OSR exit fuzzing?
> 
> Hum, it is not! I did not think about that.
> 
> Any idea how to test this properly? I want to enforce that we no longer OSR
> Exit like crazy.
> Maybe I should just have a function to tell this particular block should not
> be subject to fuzzing.
You could do that for this test since you don't really want OSR exit fuzzing to
take place for this block, since you want to enforce that we compile a block
that doesn't exit at all. I'm not sure what's the nicest way to write such a function.
I guess you could flip a bit on the UnlinkedCodeBlock, that way, every CodeBlock
can ask its UnlinkedCodeBlock if it partakes in OSR exit fuzzing.

[Benjamin Poulain]

Created attachment 286499 [details]
Patch

[Saam Barati]

Comment on attachment 286499 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:198
> +        || canUseOSRExitFuzzing(m_jit.graph().baselineCodeBlockFor(m_origin.semantic)))

I think you want:
if (!canUseOSRExitFuzzing(m_jit.graph().baselineCodeBlockFor(m_origin.semantic)) || !doOSRExitFuzzing()) as the condition here.
It's probably better for canUseOSRExitFuzzing to come first so doOSRExitFuzzing won't perform its side effects if canUseOSRExitFuzzing is false.
You also want to negate the condition otherwise you'll never emit checks when fuzzing is enabled.

[Benjamin Poulain]

Created attachment 286503 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286503 [details]
Patch

Clearing flags on attachment: 286503

Committed r204670: <http://trac.webkit.org/changeset/204670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.