BaselineCompiler: Compile remaining missing opcodes

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

Other Branch
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Posted patch patchSplinter Review
The attached patch compiles all remaining opcodes that JM handles but Baseline doesn't, so that the two compilers handle the exact same set of opcodes.  The new ones are:

JSOP_IN
JSOP_GETXPROP
JSOP_GETINTRINSIC
JSOP_CALLINTRINSIC
JSOP_INSTANCEOF
JSOP_TYPEOF
JSOP_TYPEOFEXPR

Most of these need ICs, though GETXPROP, CALLINTRINSIC and TYPEOFEXPR can reuse ICs for other ops.
Attachment #711841 - Flags: review?(kvijayan)
Comment on attachment 711841 [details] [diff] [review]
patch

Review of attachment 711841 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/BaselineIC.cpp
@@ +2354,5 @@
> +    RootedObject obj(cx, &objValue.toObject());
> +
> +    JSBool cond = false;
> +    if (!OperatorIn(cx, key, obj, &cond))
> +        return false;

I think this is an existing issue with Ion, but OperatorIn is slightly divergent from the interpreter implementation.

Interpreter uses |ValueToId| defined in jsatominlines.h, while OperatorIn uses |FetchElementId| defined in jsinterpinlines.h.

The implementations of |ValueToId| and |FetchElementId| are basically identical, despite being in two different header files.  I think it'd be good to refactor this now and land it on m-i before proceeding with the changes in baseline.

@@ +2573,5 @@
> +
> +bool
> +ICGetIntrinsic_Constant::Compiler::generateStubCode(MacroAssembler &masm)
> +{
> +    masm.loadValue(Address(BaselineStubReg, offsetof(ICGetIntrinsic_Constant, value_)), R0);

Nit: make ICGetIntrinsic_Contant::offsetOfValue() method for this.  Keeps it in line with convention on other stubs.

@@ +3848,5 @@
> +                     MutableHandleValue res)
> +{
> +    FallbackICSpew(cx, stub, "InstanceOf");
> +
> +    if (!rhs.isObject()) {

Would be good to refactor jsinterp.cpp's handling of JSOP_INSTANCEOF first into a reusable method, and then have the fallback stub reuse that.

There are slight divergences creeping in here as well (e.g. here it's "!rhs.isObject()", in jsinterp.cpp it's "rhs.isPrimitive()").

Good to refactor and land on m-i as with above.
Attachment #711841 - Flags: review?(kvijayan) → review+
(Reporter)

Comment 2

6 years ago
Filed bug 839526 for general interpreter related cleanup.

https://hg.mozilla.org/projects/ionmonkey/rev/d62ba56adc27
(Reporter)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.