Closed Bug 696825 Opened 10 years ago Closed 10 years ago

IonMonkey: Add yet more features into the ARM backend

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mjrosenb, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

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
Attachment #569127 - Flags: review?(cdleary)
Attachment #569147 - Flags: review?(cdleary)
Attached patch Implement jump tables (obsolete) — Splinter Review
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.
Attachment #569151 - Flags: review?(cdleary)
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.
Attachment #569127 - Flags: review?(cdleary) → review+
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.
Attachment #569147 - Flags: review?(cdleary) → review+
the code generation for jump tables now includes an explanation of how it works.  Kudos if you can figure it out without the comments.
Attachment #569151 - Attachment is obsolete: true
Attachment #569151 - Flags: review?(cdleary)
Attachment #571465 - Flags: review?(cdleary)
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?
Attachment #571465 - Flags: review?(cdleary) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 817960
You need to log in before you can comment on or make changes to this bug.