Closed Bug 839501 Opened 11 years ago Closed 11 years ago

BaselineCompiler: Compile remaining missing opcodes

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bhackett1024, Unassigned)

References

Details

Attachments

(1 file)

Attached 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+
Filed bug 839526 for general interpreter related cleanup.

https://hg.mozilla.org/projects/ionmonkey/rev/d62ba56adc27
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: