1667037 - Warp: Transpile CompareObjectUndefinedNullResult

Status: RESOLVED DUPLICATE of bug 1667244

(Core :: JavaScript Engine: JIT, task, P1)

Description

[André Bargull [:anba]]

Browsing (*) with IONFLAGS=warp-transpiler to detect ops which are widely used shows that CompareObjectUndefinedNullResult is a top-contender for transpilation.
Caveat: 499 of the 866 for CompareObjectUndefinedNullResult were on "www.redditstatic.com".

(*) Websites visited: wikipedia.org, reddit.com, cnn.com, youtube.com, google.com, instagram.com, amazon.com

CacheIR op	Count
CompareObjectUndefinedNullResult	866
GuardHasGetterSetter	802
LoadDOMExpandoValueGuardGeneration	706
GuardAndGetIterator	448
LoadDOMExpandoValue	407
GuardIndexGreaterThanDenseInitLength	145
CallStringObjectConcatResult	24
LoadDOMExpandoValueIgnoreGeneration	17
LoadDenseElementHoleExistsResult	16
CallClassHook	15
GuardIsNativeObject	12
GuardFunctionIsNonBuiltinCtor	8

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1667037 - Part 1: Remove strict equality code for CompareObjectUndefinedNullResult. r=jandem!

CompareObjectUndefinedNullResult is only used for loose equality.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1667037 - Part 2: Transpile CompareObjectUndefinedNullResult. r=jandem!

Depends on D91252

Comment 3

[Tom S]

You should really come on Matrix sometimes. I was just looking into comparisons with undefined/null as well. I think we really need to handle at least {null, undefined, object} x {null, undef} in one stub to make it easier to transpile. That would be useful for functions like this that are called either with undefined or some object.

function f (x) {
   if (x  === undefined) { x = {bla: 1}; }
}

We should also do this for strict equality. Otherwise we probably end up with two "active" stubs: StrictNullUndefinedEquality and StrictDifferentTypes.

Comment 4

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #3)

I was just looking into comparisons with undefined/null as well.

Whoops. My patch doesn't do anything fancy, so hopefully there shouldn't be any duplicated work. Do you want me to withdraw both patches, if they end up being superseded by your work anyway?

I think we really need to handle at least {null, undefined, object} x {null, undef} in one stub to make it easier to transpile. [...]
We should also do this for strict equality. Otherwise we probably end up with two "active" stubs: StrictNullUndefinedEquality and StrictDifferentTypes.

Yes, that sounds reasonable. I've also seen this issue for RegExp built-ins, where RegExpMatch returns null or object and callers compare with matchResult === null. In my dataset, StrictEq and StrictNe where among the JSOps with the most multiple active stubs:

Op	Count (>1000)
JSOp::Call	1206
JSOp::CallProp	1291
JSOp::GetElem	2584
JSOp::GetProp	9467
JSOp::SetProp	1003
JSOp::StrictEq	3657
JSOp::StrictNe	3986

Comment 5

[Tom S]

I think we can just land the second patch, because the Ion code can already handle more undefined comparisons. I am going to try and extend CompareObjectUndefinedNullResult to also handle undefined == undefined.

Comment 6

[Tom S]

See bug 1667037.

Comment 7

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #6)

See bug 1667037.

Bug 1667244 replaces CompareObjectUndefinedNullResult, so I think I'll withdraw both patches and we can close this bug as a dup of bug 1667244.