Bug 182216 - [ESNext][BigInt] Implement "~" unary operation
Summary: [ESNext][BigInt] Implement "~" unary operation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: InRadar
: 190798 (view as bug list)
Depends on: 190923 192966
Blocks: 179001 179900
  Show dependency treegraph
 
Reported: 2018-01-27 10:10 PST by Caio Lima
Modified: 2019-03-11 10:24 PDT (History)
11 users (show)

See Also:


Attachments
WIP - Patch (24.67 KB, patch)
2018-12-03 09:14 PST, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (25.92 KB, patch)
2018-12-04 05:53 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (810.84 KB, application/zip)
2018-12-04 08:09 PST, Build Bot
no flags Details
Patch (25.94 KB, patch)
2018-12-13 04:26 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (25.83 KB, patch)
2018-12-13 06:10 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (32.12 KB, patch)
2018-12-25 08:26 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (32.15 KB, patch)
2019-01-15 13:21 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks (94.96 KB, text/plain)
2019-01-15 17:59 PST, Caio Lima
no flags Details
Patch (32.12 KB, patch)
2019-01-15 18:06 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (35.64 KB, patch)
2019-01-23 07:59 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks (95.61 KB, text/plain)
2019-01-23 08:32 PST, Caio Lima
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.82 MB, application/zip)
2019-01-23 09:56 PST, Build Bot
no flags Details
Patch (34.91 KB, patch)
2019-01-24 05:13 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmarks (96.13 KB, text/plain)
2019-01-24 05:27 PST, Caio Lima
no flags Details
Patch (34.89 KB, patch)
2019-02-01 17:40 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (34.92 KB, patch)
2019-02-17 14:03 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (37.23 KB, patch)
2019-02-27 07:05 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (40.15 KB, patch)
2019-03-07 08:52 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (40.08 KB, patch)
2019-03-08 04:28 PST, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-01-27 10:10:31 PST
...
Comment 1 Caio Lima 2018-10-22 11:45:52 PDT
*** Bug 190798 has been marked as a duplicate of this bug. ***
Comment 2 Caio Lima 2018-12-03 09:14:15 PST
Created attachment 356382 [details]
WIP - Patch
Comment 3 Caitlin Potter (:caitp) 2018-12-03 09:32:40 PST
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 4 Build Bot 2018-12-03 10:46:32 PST
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
Comment 5 Caio Lima 2018-12-04 05:53:59 PST
Created attachment 356489 [details]
WIP - Patch
Comment 6 Build Bot 2018-12-04 08:09:32 PST
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.
Comment 7 Build Bot 2018-12-04 08:09:34 PST
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
Comment 8 Caio Lima 2018-12-13 04:26:51 PST
Created attachment 357224 [details]
Patch
Comment 9 Caio Lima 2018-12-13 06:10:11 PST
Created attachment 357227 [details]
Patch
Comment 10 Saam Barati 2018-12-18 23:49:04 PST
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
Comment 11 Caio Lima 2018-12-19 03:44:46 PST
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
Comment 12 Saam Barati 2018-12-22 15:37:16 PST
(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
Comment 13 Caio Lima 2018-12-23 01:42:43 PST
(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.
Comment 14 Caio Lima 2018-12-25 08:26:31 PST
Created attachment 358064 [details]
Patch
Comment 15 Build Bot 2018-12-25 08:29:04 PST
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 16 Robin Templeton 2019-01-14 19:36:25 PST
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
Comment 17 Caio Lima 2019-01-15 13:21:23 PST
Created attachment 359198 [details]
Patch
Comment 18 Caio Lima 2019-01-15 17:59:13 PST
Created attachment 359235 [details]
Benchmarks

This patch seems perf neutral when running on macOS x86_64.
Comment 19 Caio Lima 2019-01-15 18:06:25 PST
Created attachment 359236 [details]
Patch
Comment 20 Saam Barati 2019-01-20 13:00:56 PST
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 21 Robin Templeton 2019-01-20 13:10:12 PST
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
Comment 22 Caio Lima 2019-01-23 07:59:27 PST
Created attachment 359885 [details]
Patch
Comment 23 Caio Lima 2019-01-23 08:32:57 PST
Created attachment 359890 [details]
Benchmarks

It seems perf neutral.
Comment 24 Build Bot 2019-01-23 09:56:57 PST
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
Comment 25 Build Bot 2019-01-23 09:56:59 PST
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
Comment 26 Caio Lima 2019-01-24 05:13:33 PST
Created attachment 360006 [details]
Patch
Comment 27 Caio Lima 2019-01-24 05:27:43 PST
Created attachment 360007 [details]
Benchmarks

Changes are perf neutral
Comment 28 Caio Lima 2019-01-28 15:11:35 PST
Ping review.
Comment 29 Caio Lima 2019-02-01 17:40:48 PST
Created attachment 360931 [details]
Patch
Comment 30 Caio Lima 2019-02-05 16:58:21 PST
Pong Review
Comment 31 Caio Lima 2019-02-17 14:03:12 PST
Created attachment 362250 [details]
Patch
Comment 32 Caio Lima 2019-02-27 07:05:48 PST
Created attachment 363089 [details]
Patch
Comment 33 Caio Lima 2019-02-28 10:00:07 PST
Ping review
Comment 34 Caio Lima 2019-03-06 13:07:57 PST
Ping Review
Comment 35 Keith Miller 2019-03-06 23:20:05 PST
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 36 Caio Lima 2019-03-07 08:47:16 PST
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.
Comment 37 Caio Lima 2019-03-07 08:52:44 PST
Created attachment 363878 [details]
Patch
Comment 38 Keith Miller 2019-03-07 10:44:39 PST
(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.
Comment 39 Keith Miller 2019-03-07 19:46:51 PST
(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.
Comment 40 Caio Lima 2019-03-08 04:02:35 PST
(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.
Comment 41 Caio Lima 2019-03-08 04:28:12 PST
Created attachment 364004 [details]
Patch
Comment 42 Caio Lima 2019-03-11 06:14:51 PDT
(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?
Comment 43 Keith Miller 2019-03-11 09:38:00 PDT
(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.
Comment 44 Caio Lima 2019-03-11 10:03:45 PDT
Thx again for the review!
Comment 45 WebKit Commit Bot 2019-03-11 10:21:47 PDT
Comment on attachment 364004 [details]
Patch

Clearing flags on attachment: 364004

Committed r242715: <https://trac.webkit.org/changeset/242715>
Comment 46 WebKit Commit Bot 2019-03-11 10:21:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Radar WebKit Bug Importer 2019-03-11 10:24:45 PDT
<rdar://problem/48771508>