734830 - IonMonkey: Add in a very basic peephole phase

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mrosenberg/patches/peephole-r0.patch

This is to work around a shortcoming of our mostly x86-centric compiler.
This looks for repeated moves of similar constant values, and collapses them.
for example, the code we presently generate for the inlining check looks like:
movw r12, 0x1234
movt r12, 0x5678
ldr r0, [r12]
adds r0, r0, 1
movw r12, 0x1234
movt r12, 0x5678
str r0, [r12]

With this patch in place, we simply eliminate the second movw/movt pair.
While there are simpler ways of getting this specific optimization, I suspect there are many instances where this will help.

Comment 1

[Jacob Bramley [:jbramley]]

Comment on attachment 604867 [details] [diff] [review]
/home/mrosenberg/patches/peephole-r0.patch

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

I think it could have been cleaner to simply integrate the peephole optimization into the MacroAssemblerARM class directly, but I could be wrong. Some of the implementation that is (now) in Peephole-arm.cpp doesn't actually seem to do any optimizing, but I suspect that you intend to extend it further.

::: js/src/ion/Ion.cpp
@@ +1122,1 @@
>          InvalidateActivation(cx, iter.top(), false);

I can't see what 'activations' is used for.

@@ +1167,5 @@
> +} // ion
> +} // js
> +static uint32 stopOPT = UINT32_MAX;
> +void
> +ion::doOpt()

This function doesn't actually do anything except print a spew message (and check for stopOPT). Perhaps it should be called 'logOpt' or 'countOpt' or something like that.

@@ +1171,5 @@
> +ion::doOpt()
> +{
> +    if (stopOPT == optCount)
> +        dbg_break();
> +    IonSpew(IonSpew_Opts, "Optimization number %d", optCount);

Use %u, not %d, to print a uint32.

::: js/src/ion/Ion.h
@@ +107,5 @@
>      // Default: 10,240
>      uint32 usesBeforeInlining;
>  
> +    // How many optimizations should be performed?
> +    // UINT_MAX is infinite, and the default

What about it? (Unfinished comment.)

@@ +108,5 @@
>      uint32 usesBeforeInlining;
>  
> +    // How many optimizations should be performed?
> +    // UINT_MAX is infinite, and the default
> +    uint32 maxOpts;

How does this differ from stopOPT?

@@ +184,5 @@
>  
> +static inline bool canOpt()
> +{
> +#ifdef DEBUG
> +    extern uint32 optCount;

Can't this be a field on a class somewhere, rather than a global-ish variable?

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +189,5 @@
>  }
>  void
>  MacroAssemblerARM::ma_rol(Register shift, Register src, Register dst)
>  {
> +

Extra whitespace.

@@ +308,5 @@
>  }
>  void
>  MacroAssemblerARM::ma_adc(Register src, Register dest, SetCond_ sc, Condition c)
>  {
>      as_alu(dest, dest, O2Reg(src), op_adc, sc, c);

Why doesn't this get to the peephole optimizer? (Also, the one after it.)

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +55,5 @@
>  
>  static Register CallReg = ip;
>  static const int defaultShift = 3;
>  JS_STATIC_ASSERT(1 << defaultShift == sizeof(jsval));
>  // MacroAssemblerARM is inheriting form Assembler defined in Assembler-arm.{h,cpp}

This comment is now out-of-date.

::: js/src/ion/arm/Peephole-arm.h
@@ +51,5 @@
> +        bool isKnown_;
> +        ConstReg() : value_(0), isKnown_(false)
> +        {
> +        }
> +        void setReg(uint32 v) {

'setValue' (and 'trashValue') would line up better with 'hasValue'.

@@ +60,5 @@
> +        void trashReg() {
> +            isKnown_ = false;
> +        }
> +
> +        bool hasValue(uint32 *dest) {

'dest' is probably not the best name here. 'v' or 'value' would be better.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +349,5 @@
>      MacroAssembler masm(cx);
>      // ArgumentsRectifierReg contains the |nargs| pushed onto the current frame.
>      // Including |this|, there are (|nargs| + 1) arguments to copy.
>      JS_ASSERT(ArgumentsRectifierReg == r8);
> +    

Extra whitespace.

Comment 2

[Chris Peterson [:cpeterson]]

Marty, is this ARM peephole patch still relevant? Jacob r+'d over a year ago. <:)

Comment 3

[Marty Rosenberg [:mjrosenb]]

Nothing has implemented this functionality, but when I wrote it, I couldn't measure a difference with and without the patch.  It should be re-tested at some point.