Bug 182216

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

Created attachment 356382 [details] WIP - Patch

Comment on attachment 356382 [details] WIP - Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356382&action=review I've given this a quick look --- I don't feel too good reviewing DFG/FTL stuff as it's been a while and I don't have it fresh in my mind, so the style nit is all I got there. The runtime looks good and matches the v8 implementation --- unfortunately I'm not super familiar with the spec and will have a hard time vetting the tests, maybe Robin could take a look at those? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:387 > + if (node->op() == ValueBitNot) { do we really need to fall through ValueBitNot into this? the amount of shared code is pretty small

Comment on attachment 356382 [details] WIP - Patch Attachment 356382 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10250201 New failing tests: exceptionFuzz.yaml/exceptionFuzz/earley-boyer.js.exception-fuzz apiTests

Created attachment 356489 [details] WIP - Patch

Comment on attachment 356489 [details] WIP - Patch Attachment 356489 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10264412 Number of test failures exceeded the failure limit.

Created attachment 356498 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 357224 [details] Patch

Created attachment 357227 [details] Patch

Comment on attachment 357227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357227&action=review Mostly LGTM, but I’m worried about how you lower to ArithBitNot vs ValueBitNot > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:383 > + setConstant(node, JSValue(~a)); I think you need a didFoldClobber here > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4929 > + if (isInt32Speculation(getPrediction())) This reasoning is wrong. You’re saying you’ll produce an ArithBitNot if you produce an int32, but you’ll always produce an Int32 I believe if you’re not a big int. This logic should consider the operands, not the result. I think this kind of logic is best done in Fixup. If you kept this code this way, I believe you’re introducing an OSR exit loop every time you have some object type as an input > Source/JavaScriptCore/dfg/DFGOperations.cpp:368 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); No need for this extra branch. I think you can just RELEASE_AND_RETURN the above operation

Thank you for the review. (In reply to Saam Barati from comment #10) > Comment on attachment 357227 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357227&action=review > > Mostly LGTM, but I’m worried about how you lower to ArithBitNot vs > ValueBitNot > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:383 > > + setConstant(node, JSValue(~a)); > > I think you need a didFoldClobber here > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4929 > > + if (isInt32Speculation(getPrediction())) > > This reasoning is wrong. You’re saying you’ll produce an ArithBitNot if you > produce an int32, but you’ll always produce an Int32 I believe if you’re not > a big int. This logic should consider the operands, not the result. I think > this kind of logic is best done in Fixup. If you kept this code this way, I > believe you’re introducing an OSR exit loop every time you have some object > type as an input Maybe I'm missing some edge case here, but I don't understand why such situation would be possible. Isn't this the same situation as the test `JSTests/stress/bitwise-not-fixup-rules.js` is stressing? Even when we emit ArithBitNot we are fixing this for ValueBitNot into fixup phase when operand should be Untyped. BTW, this is the same approach we are following in all other bitwise operations. > > Source/JavaScriptCore/dfg/DFGOperations.cpp:368 > > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > > No need for this extra branch. I think you can just RELEASE_AND_RETURN the > above operation

(In reply to Caio Lima from comment #11) > Thank you for the review. > > (In reply to Saam Barati from comment #10) > > Comment on attachment 357227 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=357227&action=review > > > > Mostly LGTM, but I’m worried about how you lower to ArithBitNot vs > > ValueBitNot > > > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:383 > > > + setConstant(node, JSValue(~a)); > > > > I think you need a didFoldClobber here > > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4929 > > > + if (isInt32Speculation(getPrediction())) > > > > This reasoning is wrong. You’re saying you’ll produce an ArithBitNot if you > > produce an int32, but you’ll always produce an Int32 I believe if you’re not > > a big int. This logic should consider the operands, not the result. I think > > this kind of logic is best done in Fixup. If you kept this code this way, I > > believe you’re introducing an OSR exit loop every time you have some object > > type as an input > > Maybe I'm missing some edge case here, but I don't understand why such > situation would be possible. Isn't this the same situation as the test > `JSTests/stress/bitwise-not-fixup-rules.js` is stressing? > Even when we emit ArithBitNot we are fixing this for ValueBitNot into fixup > phase when operand should be Untyped. BTW, this is the same approach we are > following in all other bitwise operations. You’re correct. I missed the code in Fixup that does this. I guess it’s weird to first start with ArithBitNot and then move to ValueBitNot. This probably doesn’t have an effect in practice, but it’s subtly wrong if we ever move some analyses before Fixup. As a general rule of thumb, you can change a node from performing effects to not performing effects. However, taking a node that doesn’t perform effects and converting it to a node that does effects is wrong. It may not matter this early in the compiler pipeline, but I think we should follow this rule. E.g, we use clobberize rules in BytecodeParser to report if it’s valid to exit at a subsequent node. We should start by lowering to ValueBitNot, then in if possible, in Fixup convert to ArithBitNot and clear the must generate flags > > > > Source/JavaScriptCore/dfg/DFGOperations.cpp:368 > > > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > > > > No need for this extra branch. I think you can just RELEASE_AND_RETURN the > > above operation

(In reply to Saam Barati from comment #12) > You’re correct. I missed the code in Fixup that does this. I guess it’s > weird to first start with ArithBitNot and then move to ValueBitNot. This > probably doesn’t have an effect in practice, but it’s subtly wrong if we > ever move some analyses before Fixup. As a general rule of thumb, you can > change a node from performing effects to not performing effects. However, > taking a node that doesn’t perform effects and converting it to a node that > does effects is wrong. It may not matter this early in the compiler > pipeline, but I think we should follow this rule. E.g, we use clobberize > rules in BytecodeParser to report if it’s valid to exit at a subsequent node. After your comment I started to think about it and agree that this reasoning is wrong. I decided to fix other Nodes into https://bugs.webkit.org/show_bug.cgi?id=192966.

Created attachment 358064 [details] Patch

Attachment 358064 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:286: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 358064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358064&action=review looks good for the parts i can review, some small issues and nits below > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:398 > + case ArithBitNot: {; nit: extra semicolon > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:292 > + nit: trailing whitespace > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2891 > + nit: trailing whitespace > Source/JavaScriptCore/runtime/JSBigInt.cpp:540 > + return result->rightTrim(vm); this should check for errors from absoluteSubOne/absoluteAddOne before trimming (also both of those functions appear to trim their results, so calling rightTrim may not be strictly necessary here) > JSTests/stress/big-int-bitwise-not-wrapped-value.js:21 > +testBitNot(o, -2n, "ToPrimitive: @@toPrimitive"); this could also check that toPrimitive/valueOf/toString are prioritized correctly by defining the unused methods and throwing errors from them: valueOf and toString in this test, and toString in the next one test262 checks for this in https://github.com/tc39/test262/blob/master/test/language/expressions/bitwise-not/bigint-non-primitive.js > JSTests/stress/bitwise-not-fixup-rules.js:1 > +//@ runBigIntEnabled might need '//@ skip if not $jitTests' -- numberOfDFGCompiles will return a large number if the jit is not enabled

Created attachment 359198 [details] Patch

Created attachment 359235 [details] Benchmarks This patch seems perf neutral when running on macOS x86_64.

Created attachment 359236 [details] Patch

Comment on attachment 359236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359236&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:383 > + didFoldClobberWorld(); I think you only want to call this if you're UntypedUse. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:242 > + if (operandEdge.node()->shouldSpeculateBigInt()) > + fixEdge<BigIntUse>(operandEdge); I think you can clear mustGenerate here too, right? This isn't observable IIRC

Comment on attachment 359236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359236&action=review tests and runtime code lgtm > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4938 > + set(bytecode.dst, addToGraph(ArithBitNot, op1)); nit: bytecode.dst -> bytecode.m_dst in master (from bug 193467?) > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:725 > + auto operandNumeric = GET_C(bytecode.operand).jsValue().toBigIntOrInt32(exec); and bytecode.operand -> bytecode.m_operand

Created attachment 359885 [details] Patch

Created attachment 359890 [details] Benchmarks It seems perf neutral.

Comment on attachment 359885 [details] Patch Attachment 359885 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10859239 New failing tests: imported/w3c/web-platform-tests/css/css-grid/abspos/orthogonal-positioned-grid-descendants-013.html

Created attachment 359898 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Created attachment 360006 [details] Patch

Created attachment 360007 [details] Benchmarks Changes are perf neutral

Ping review.

Created attachment 360931 [details] Patch

Pong Review

Created attachment 362250 [details] Patch

Created attachment 363089 [details] Patch

Ping review

Ping Review

Comment on attachment 363089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363089&action=review r=me with some comments. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:384 > + if (node->child1().useKind() == UntypedUse) > + didFoldClobberWorld(); Why do you need this check? It doesn't seem like you should need it because even if Fixup thinks this should have been a BigInt, AI has proven it's an int32. > Source/JavaScriptCore/dfg/DFGOperations.cpp:216 > +template<typename BigIntOperation, typename Int32Operation> > +static ALWAYS_INLINE EncodedJSValue bitwiseBinaryOp(ExecState* exec, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2, BigIntOperation&& bigIntOp, Int32Operation&& int32Op, const char* errorMessage) Nice! > JSTests/stress/big-int-bit-not-general.js:4 > +// Copyright (C) 2017 Josh Wolfe. All rights reserved. > +// This code is governed by the BSD license found in the LICENSE file. Does this file exist? If not please paste the contents of the original BSD license here.

Comment on attachment 363089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363089&action=review Thx a lot for the review! >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:384 >> + didFoldClobberWorld(); > > Why do you need this check? It doesn't seem like you should need it because even if Fixup thinks this should have been a BigInt, AI has proven it's an int32. I agree with you. I can't think in a case where it is reproducible, but we are getting the useKind of `node->child1()` and it can be anything. AFICT, there is no way this happen, but to be robust against future changes, instead of removing the condition, I added an ASSERT(node->child1().useKind() == UntypedUse). >> JSTests/stress/big-int-bit-not-general.js:4 >> +// This code is governed by the BSD license found in the LICENSE file. > > Does this file exist? If not please paste the contents of the original BSD license here. Done.

Created attachment 363878 [details] Patch

(In reply to Caio Lima from comment #36) > Comment on attachment 363089 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363089&action=review > > Thx a lot for the review! > > >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:384 > >> + didFoldClobberWorld(); > > > > Why do you need this check? It doesn't seem like you should need it because even if Fixup thinks this should have been a BigInt, AI has proven it's an int32. > > I agree with you. I can't think in a case where it is reproducible, but we > are getting the useKind of `node->child1()` and it can be anything. AFICT, > there is no way this happen, but to be robust against future changes, > instead of removing the condition, I added an > ASSERT(node->child1().useKind() == UntypedUse). That’s definitely not a valid assertion. Prediction the value profile coming into the not can say anything it wants. This includes intentionally lying, which we sometimes do for testing. It’s basically a guess. Thus, it’s perfectly valid for Fixup to decide to check for a JSBigInt. However AI will only say a value is an int32 if it’s 100% true. So it’s completely possible for us to have a JSBigInt use kind and have AI prove it’s an int32. > > >> JSTests/stress/big-int-bit-not-general.js:4 > >> +// This code is governed by the BSD license found in the LICENSE file. > > > > Does this file exist? If not please paste the contents of the original BSD license here. > > Done.

(In reply to Keith Miller from comment #38) > (In reply to Caio Lima from comment #36) > > Comment on attachment 363089 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=363089&action=review > > > > Thx a lot for the review! > > > > >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:384 > > >> + didFoldClobberWorld(); > > > > > > Why do you need this check? It doesn't seem like you should need it because even if Fixup thinks this should have been a BigInt, AI has proven it's an int32. > > > > I agree with you. I can't think in a case where it is reproducible, but we > > are getting the useKind of `node->child1()` and it can be anything. AFICT, > > there is no way this happen, but to be robust against future changes, > > instead of removing the condition, I added an > > ASSERT(node->child1().useKind() == UntypedUse). > > That’s definitely not a valid assertion. Prediction the value profile coming > into the not can say anything it wants. This includes intentionally lying, > which we sometimes do for testing. It’s basically a guess. Thus, it’s > perfectly valid for Fixup to decide to check for a JSBigInt. However AI will > only say a value is an int32 if it’s 100% true. So it’s completely possible > for us to have a JSBigInt use kind and have AI prove it’s an int32. > Can you please remove that assertion? > > > > >> JSTests/stress/big-int-bit-not-general.js:4 > > >> +// This code is governed by the BSD license found in the LICENSE file. > > > > > > Does this file exist? If not please paste the contents of the original BSD license here. > > > > Done.

(In reply to Keith Miller from comment #39) > (In reply to Keith Miller from comment #38) > > > > That’s definitely not a valid assertion. Prediction the value profile coming > > into the not can say anything it wants. This includes intentionally lying, > > which we sometimes do for testing. It’s basically a guess. Thus, it’s > > perfectly valid for Fixup to decide to check for a JSBigInt. However AI will > > only say a value is an int32 if it’s 100% true. So it’s completely possible > > for us to have a JSBigInt use kind and have AI prove it’s an int32. > > > > Can you please remove that assertion? Sure. Sorry for the delay on reply, but I was thinking in a way to add a test for this case before upload the patch. My major confusion here is because I'm asserting on ```node->child1.useKind``` and this is definitely not guaranteed. When AI executes ```ValueBitNot``` edges and its UseKind is ```BigIntUse``` means that AI can't prove it is a Int32 anymore, which makes sense. So the following code wouldn't propagate a constant: ... 1: @ArithOp(Int32) 2: @ValueBit(Check:BigInt:@1) 3: @return(Check:Untyped:@2) ... So, in the case of ValueBitNot, only ```UntypedUse``` is possible to allow AI prove it is Int32 ant that's why we don't need the check/assertion. Did I get it right? The problem of getting a test case for this is because Fixup will change ValueBitNot by ArithBitNot when its operand is AnyInt or Number.

Created attachment 364004 [details] Patch

(In reply to Caio Lima from comment #40) > Sure. Sorry for the delay on reply, but I was thinking in a way to add a > test for this case before upload the patch. My major confusion here is > because I'm asserting on ```node->child1.useKind``` and this is definitely > not guaranteed. When AI executes ```ValueBitNot``` edges and its UseKind is > ```BigIntUse``` means that AI can't prove it is a Int32 anymore, which makes > sense. So the following code wouldn't propagate a constant: > > ... > 1: @ArithOp(Int32) > 2: @ValueBit(Check:BigInt:@1) > 3: @return(Check:Untyped:@2) > ... > > So, in the case of ValueBitNot, only ```UntypedUse``` is possible to allow > AI prove it is Int32 ant that's why we don't need the check/assertion. Did I > get it right? > > The problem of getting a test case for this is because Fixup will change > ValueBitNot by ArithBitNot when its operand is AnyInt or Number. I get it now. `didFoldClobberWorld` needs to align with clobberize rules, and as I explained before, BigIntUse on ValueBitNot won't allow AI proves an Int32 constant. The assertion is indeed not needed and is wrong because it is not related with child's use kind. I think we are good to commit current Patch, right?

(In reply to Caio Lima from comment #42) > (In reply to Caio Lima from comment #40) > > Sure. Sorry for the delay on reply, but I was thinking in a way to add a > > test for this case before upload the patch. My major confusion here is > > because I'm asserting on ```node->child1.useKind``` and this is definitely > > not guaranteed. When AI executes ```ValueBitNot``` edges and its UseKind is > > ```BigIntUse``` means that AI can't prove it is a Int32 anymore, which makes > > sense. So the following code wouldn't propagate a constant: > > > > ... > > 1: @ArithOp(Int32) > > 2: @ValueBit(Check:BigInt:@1) > > 3: @return(Check:Untyped:@2) > > ... > > > > So, in the case of ValueBitNot, only ```UntypedUse``` is possible to allow > > AI prove it is Int32 ant that's why we don't need the check/assertion. Did I > > get it right? > > > > The problem of getting a test case for this is because Fixup will change > > ValueBitNot by ArithBitNot when its operand is AnyInt or Number. > > I get it now. `didFoldClobberWorld` needs to align with clobberize rules, > and as I explained before, BigIntUse on ValueBitNot won't allow AI proves an > Int32 constant. The assertion is indeed not needed and is wrong because it > is not related with child's use kind. I think we are good to commit current > Patch, right? Whoops, thought I commented before... I was actually wrong and the original patch was fine. As you said, there's no way for AI to prove int32 with BitIntUse because it will reach a contradiction. I'm OK with either version of this patch.

Thx again for the review!

Comment on attachment 364004 [details] Patch Clearing flags on attachment: 364004 Committed r242715: <https://trac.webkit.org/changeset/242715>

All reviewed patches have been landed. Closing bug.

<rdar://problem/48771508>