516760 - optimize OP_istypelate when RHS type is known

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), enhancement, P3)

Description

[Edwin Smith]

a typical use case is

   (x is T)

where T is the name of a class.  When the JIT compiles the code, we'll know the operand is type T$ (the T class object) and we can optimize the call to use the (constant) instance type of T.

Comment 1

[Edwin Smith]

Attachment: when we know the RHS type, use the faster form of istype

- simplifies the code in CodegenLIR by not calling emit() from write().
- istype and istype_late return int (really, bool) instead of Atom
- when the RHS type of istypelate is a class then precompute the constant itraits.

ISSUE: when the compile time type is T$?  (i.e. T class object, or null), then the runtime RHS might be null, which triggers an exception.  folding the itraits in would avoid the error.

this could only happen if you executed istypelate val, T, at a point before class T had been initialized.  Inserting the constant itraits for T instead of using the runtime value for T could even delay the point when T gets initialized.  (if this is T's first reference, for example).

imo these are really language / asc bugs.  but the optimization as-patched is not quite legal because of these possibilities.

Comment 2

[Steven Johnson]

Comment on attachment 401143 [details] [diff] [review]
when we know the RHS type, use the faster form of istype

If AvmCore::istypeAtom is no longer in use, let's nuke it.

Not sure if we want to land this unless/until we can convince ourselves that the not-quite-legal bit you pointed out is a nonissue.

Comment 3

[Edwin Smith]

istypeAtom() is used by interpreter, but we could move it there where it belongs, probably.

Comment 4

[Edwin Smith]

Attachment: Rebased patch

A workaround to the init-semantics-change problem is to artificially preserve the call to finddef():

    istypelate(obj, getprop(finddef(C),C))

becomes

    finddef(C)
    istype(obj, C-traits)

The speedup is less than if we remove finddef but it lets us break the dependency this optimization has on the loading semantics, and separately we can optimize out the finddef when its provable that the object is already initialized.

(background refresher:  if the language would specify that the expression (obj is C) won't cause class C to be initialized when C is a known compile-time class type, then this all is moot and we can elide the finddef() call with no checks.

its really an extreme edge case.  (obj is C) cannot be true if C isn't initialized yet.

Comment 5

[Steven Johnson]

I'd be really curious to know if we can find any extant SWFs that rely on this behavior. (Not sure how we could track 'em down, but...)

Comment 6

[Edwin Smith]

Attachment: final updated and rebased patch

Final candidate.

This one adds a parameter to the optimized OP_istypelate helper that takes the class object, which is subsequently ignored, to ensure that any side effects from calling finddef are preserved.

also, the helper for OP_istype (compiler-provided type) has no side effects under any conditions, so it can be a CSEFUNCTION.

Comment 7

[Steven Johnson]

Comment on attachment 408458 [details] [diff] [review]
final updated and rebased patch

style nit: predominate style for function braces in tamarin is

    int foo() 
    {
    }

rather than

    int foo() {
    }

Comment 8

[Edwin Smith]

correct, will fix braces.

Comment 9

[Edwin Smith]

(In reply to comment #1)

> ISSUE: when the compile time type is T$?  (i.e. T class object, or null), then
> the runtime RHS might be null, which triggers an exception.  folding the
> itraits in would avoid the error.

Sigh, i just realized the latest patch, while preserving any calls to finddef, still doesn't preserve this error behavior.  To do that, istypelate_known() still needs a null check on the type value.

Comment 10

[Lars T Hansen]

Comment on attachment 408458 [details] [diff] [review]
final updated and rebased patch

To the best of my ability...

Comment 11

[Edwin Smith]

http://hg.mozilla.org/tamarin-redux/rev/35141c363ac6

Comment 12

[Chris Peyer]

Engineering work item.  Marking as verified.