696825 - IonMonkey: Add yet more features into the ARM backend

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: Add in double pools

Three separate patches from my work last week:
Place doubles into pools so that we can actually encode all 2^64-2^17 doubles
Implement jump tables (this on was easier than I expected, but there is a bunch of junk code laying around.)
Implement a bunch of other easier opcodes

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: Add in miscelanous opcodes + fixes

Comment 2

[Marty Rosenberg [:mjrosenb]]

Attachment: Implement jump tables

I just realized that this one isn't complete.  I'd wanted to add in a large comment explaining what exactly the generated code is doing. It is incredibly hackish at present.  Feel free to look at and comment on everything else though.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 569127 [details] [diff] [review]
Add in double pools

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

::: js/src/assembler/assembler/AssemblerBufferWithConstantPool.h
@@ +202,5 @@
>      }
>  
> +    // places 1 int worth of data into a pool, and mashes an instruction into place to
> +    // hold this offset.
> +    // the caller of putIntWithConstantInt passes in some code thatrepresents an

Instead of "passes in some code" do you mean passes in |insn|? We usually refer to params by name if it makes the comment easier. (Also typo on 'thatrepresents'.)

::: js/src/ion/arm/Assembler-arm.cpp
@@ +120,5 @@
> +    // up in JM.
> +    // TODO: implement this!
> +#if 0
> +    fixUpOffsets(buffer);
> +#endif

Bug number for '#if 0's and 'FIXME's are customary. (Also address the commented out bits below, please.)

@@ +803,5 @@
> +};
> +typedef union PoolHintPun {
> +    PoolHintData phd;
> +    uint32 raw;
> +} PoolHintPun;

No need for typedef, it's C++! :-)

@@ +913,5 @@
> +
> +uint32
> +Assembler::placeConstantPoolBarrier(int offset)
> +{
> +    JS_NOT_REACHED("ARMAssembler holdover");

Could we add a FIXME/bug# to say that this is still a reachable path, but we don't hit it in the test suite as of yet?

::: js/src/ion/arm/Assembler-arm.h
@@ +909,5 @@
>      ARMBuffer m_buffer;
> +    // was the last instruction emitted an unconditional branch?
> +    // if it was, then this is a good candidate for dumping the pool!
> +    bool lastWasUBranch;
> +    static Assembler *dummy;

Comment explaining why this exists, please.

@@ +948,5 @@
>      // Size of the data table, in bytes.
>      size_t dataSize() const;
>      size_t bytesNeeded() const;
>      // write a blob of binary into the instruction stream
> +    void writeInst(uint32 x, uint32 * dest = NULL);

Comment on what |dest| is used for, please. Maybe somewhere common (like the header of the class) because it's used in a bunch of places.

@@ +1259,5 @@
>      };
> +    // generate an initial placeholder instruction that we want to later fix up
> +    static uint32 patchConstantPoolLoad(uint32 load, int32 value);
> +    // take the stub value that was written in before, and write in an actual load
> +    // using the nidex we'd computed previously as well as the address of the pool start.

'nidex' is a small typo.

@@ +1262,5 @@
> +    // take the stub value that was written in before, and write in an actual load
> +    // using the nidex we'd computed previously as well as the address of the pool start.
> +    static void patchConstantPoolLoad(void* loadAddr, void* constPoolAddr);
> +    static uint32 placeConstantPoolBarrier(int offset);
> +    // move our entire pool into the instruction stream

Maybe explain that just having seen a ubranch is a precondition, since this is an opportunistic constant pool dump, and reserving space takes care of the case where no ubranch is seen.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 569147 [details] [diff] [review]
Add in miscelanous opcodes + fixes

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

Sorry, lots of nits, but I care more about the consistency of style of the ion/ stuff than the assembler/ stuff.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +45,5 @@
>  using namespace js;
>  using namespace js::ion;
> +
> +// Encode a standard register when it is being used as src1, the dest, and
> +// an extra register.  These should never be called with an InvalidReg.

Uber nit: Brendan has always said we can't use french spacing in comments. (One space after periods, plz. :-)

@@ +73,5 @@
> +// register that has been omitted.
> +uint32
> +js::ion::maybeRT(Register r)
> +{
> +    if (r == InvalidReg) {

Nit: no brackets for single-line consequent (same nit for below).
https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Conditionals

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +79,5 @@
>  };
>  
>  
> +static inline Assembler::Condition
> +JSOpToCondition(JSOp op)

Cool.

@@ +191,5 @@
> +CodeGeneratorARM::emitSet(Assembler::Condition cond, const Register &dest)
> +{
> +    masm.ma_mov(Imm32(0), dest);
> +    masm.ma_mov(Imm32(1), dest,
> +                NoSetCond, cond);

Nit: fits on a single line.

@@ +204,5 @@
>  
>      masm.ma_cmp(ToRegister(left), ToOperand(right));
> +    masm.ma_mov(Imm32(0), ToRegister(def));
> +    masm.ma_mov(Imm32(1), ToRegister(def),
> +                NoSetCond, JSOpToCondition(comp->jsop()));

Ditto nit: first on a single line.

@@ +797,5 @@
>      //        then it was clamped to INT_MIN/INT_MAX, and we can test it.
>      //        NOTE: if the value really was supposed to be INT_MAX / INT_MIN
>      //        then it will be wrong.
>      // 2) convert the floating point value to an integer, if it did not fit,
>      //        then it set one or two bits in the fpcsr.  Check those.

Nit: I didn't realize this comment pertained to the function below. Could we move it down and over to the left?

@@ +1161,5 @@
> +    // shown as Overflow.
> +    masm.ma_b(ifFalse->label(), Assembler::Overflow);
> +    if (!isNextBlock(ifTrue)) {
> +        masm.ma_b(ifTrue->label());
> +    }

Nit: brackes.

@@ +1169,5 @@
> +
> +bool
> +CodeGeneratorARM::visitCompareD(LCompareD *comp)
> +{
> +

Nit: extra line.

::: js/src/ion/arm/CodeGenerator-arm.h
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +2 * vim: set ts=4 sw=4 et tw=79:

Two snuck in here.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +854,5 @@
>  
>      // higher level tag testing code
>  Assembler::Condition
> +MacroAssemblerARM::compareDoubles(JSOp compare,
> +                                  FloatRegister lhs, FloatRegister rhs)

Nit: fits on a single line.

@@ +871,5 @@
> +      default:
> +        JS_NOT_REACHED("Unrecognized comparison operation");
> +        return Assembler::Equal;
> +    }
> +

Nit: extra line.

Comment 5

[Marty Rosenberg [:mjrosenb]]

Attachment: Implement jump tables, and add comments explaining how it works

the code generation for jump tables now includes an explanation of how it works.  Kudos if you can figure it out without the comments.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 571465 [details] [diff] [review]
Implement jump tables, and add comments explaining how it works

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

::: js/src/ion/x64/Lowering-x64.cpp
@@ +195,5 @@
>      return defineReuseInput(lir, div) && assignSnapshot(lir);
>  }
>  
> +bool
> +LIRGeneratorX64::visitTableSwitch(MTableSwitch *tableswitch)

Can this be shared between x64 and x86?