[JSC] ArithSqrt should work with any argument type
Created attachment 286364 [details] Patch
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?
(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.
(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.
Created attachment 286499 [details] Patch
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.
Created attachment 286503 [details] Patch
Comment on attachment 286503 [details] Patch Clearing flags on attachment: 286503 Committed r204670: <http://trac.webkit.org/changeset/204670>
All reviewed patches have been landed. Closing bug.