Last Comment Bug 683300 - IonMonkey skeleton for ARM
: IonMonkey skeleton for ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-30 13:40 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2011-08-31 18:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
make the ion code build on arm (2.99 KB, patch)
2011-08-30 13:40 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
make ion build on arm (158.04 KB, patch)
2011-08-30 18:56 PDT, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review
now with fixes (191.68 KB, patch)
2011-08-31 16:56 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
patch, now with misc non-arm comment fixes (192.57 KB, patch)
2011-08-31 17:49 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-08-30 13:40:56 PDT
Created attachment 556978 [details] [diff] [review]
make the ion code build on arm

I've added in a very wrong skeleton for arm.  For the most part, I just copied the x86 code, #ifdef'ed out the important bits, then asserted on everything.
Comment 1 Marty Rosenberg [:mjrosenb] 2011-08-30 18:56:32 PDT
Created attachment 557067 [details] [diff] [review]
make ion build on arm

Unlike the last patch, this one has all of the new files that are being added to ion/arm.
So far, I've made sure that this builds when configured with --enable-ion.  With --disable-ion (the default for arm, still), it does not build.
no testing past a successful build has been done, since I don't expect anything to pass.
Comment 2 Sean Stangl [:sstangl] 2011-08-31 11:40:52 PDT
Comment on attachment 557067 [details] [diff] [review]
make ion build on arm

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

When applying the patch, git-apply complained that a number of lines contain trailing whitespace: please apply `sed 's/[ \t]*$//'`.

::: js/src/assembler/assembler/ARMAssembler.h
@@ +1594,5 @@
>  
>          void fcpyd_r(int dd, int dm, Condition cc = AL)
>          {
>              js::JaegerSpew(js::JSpew_Insns,
> +                           IPFX   "%-15s %s, %s, %s\n", MAYBE_PAD, "vmov.f64", MAYBE_PAD,

This looks like one too many MAYBE_PADs. If the problem was too many "%s" in the format string, could we remove one? I'm not sure what padding in the middle of output would mean.

::: js/src/assembler/assembler/AssemblerBufferWithConstantPool.h
@@ +187,4 @@
>      {
> +        // oh god, what is this doing here?
> +
> +        //        flushIfNoSpaceFor(maxInstructionSize, sizeof(uint64_t));

This function call should be reinstated -- judging by the uncheckedSize() definition below, it looks like size() is intended to guarantee space, while uncheckedSize() actually reports the current size.

::: js/src/ion/arm/Architecture-arm.h
@@ +38,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef jsion_cpu_arm_regs_h__

Probably should be "jsion_architecture_arm_h__". If the x86/x64 ones are mislabeled, they should be changed too -- we probably underwent a name change and forgot to update the header guards. The comments at the #endifs need updating too.

@@ +43,5 @@
> +#define jsion_cpu_arm_regs_h__
> +
> +#include "assembler/assembler/ARMAssembler.h"
> +#include "ion/shared/Assembler-shared.h"
> +namespace js {

Newline before 'namespace'.

@@ +73,5 @@
> +    static const char *GetName(Code code) {
> +        static const char *Names[] = { "r0", "r1", "r2", "r3",
> +                                       "r4", "r5", "r6", "r7",
> +                                       "r8", "r9", "r10", "r11",
> +                                       "r12", "r13", "r14", "pc"};

If we're calling %r13 "sp", would it be more helpful to print "sp"?

@@ +80,5 @@
> +
> +    static const Code StackPointer = JSC::ARMRegisters::sp;
> +    static const Code Invalid = JSC::ARMRegisters::invalid_reg;
> +
> +    static const uint32 Total = 8;

It looks like there are actually 16 registers.

@@ +81,5 @@
> +    static const Code StackPointer = JSC::ARMRegisters::sp;
> +    static const Code Invalid = JSC::ARMRegisters::invalid_reg;
> +
> +    static const uint32 Total = 8;
> +    static const uint32 Allocatable = 6;

This number seems off -- it should be equal to the number of set bits in the AllocatableMask.

@@ +100,5 @@
> +        (1 << JSC::ARMRegisters::r8) |
> +        (1 << JSC::ARMRegisters::r9) |
> +        (1 << JSC::ARMRegisters::r10) |
> +        (1 << JSC::ARMRegisters::r11) |
> +        (1 << JSC::ARMRegisters::r12) |

Put in comment: //JSC::ARMRegisters::r15 is JSC::ARMRegisters::sp.

@@ +101,5 @@
> +        (1 << JSC::ARMRegisters::r9) |
> +        (1 << JSC::ARMRegisters::r10) |
> +        (1 << JSC::ARMRegisters::r11) |
> +        (1 << JSC::ARMRegisters::r12) |
> +        (1 << JSC::ARMRegisters::r14);

Put in comment: // JSC::ARMRegisters::r15 is JSC::ARMRegisters::pc.

@@ +108,5 @@
> +        VolatileMask | NonVolatileMask;
> +    // we should also account for any scratch registers that we care about.x
> +    // possibly the stack as well.
> +    static const uint32 NonAllocatableMask =
> +        (1 << JSC::ARMRegisters::r13) |

JSC::ARMRegisters::sp

@@ +126,5 @@
> +                                       "d12", "d13", "d14", "d15"};
> +        return Names[code];
> +    }
> +
> +    static const Code Invalid = JSC::ARMRegisters::d0;

d0 is very much a valid register! Maybe JSC::ARMRegisters::InvalidFloatreg?

@@ +137,5 @@
> +    static const uint32 VolatileMask = AllMask;
> +    static const uint32 NonVolatileMask = 0;
> +
> +    static const uint32 NonAllocatableMask =
> +        (1 << JSC::ARMRegisters::d0);

Why isn't this allocatable? If it is being used as a scratch float, then it needs to be named "ScratchFloatReg" elsewhere, and there should be a comment explaining that this is ScratchFloatReg.

@@ +142,5 @@
> +
> +    static const uint32 AllocatableMask = AllMask & ~NonAllocatableMask;
> +};
> +} // namespace js
> +} // namespace ion

The comments are backwards -- ion's namespace ends first.

::: js/src/ion/arm/Assembler-arm.h
@@ +38,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef jsion_cpu_arm_assembler_h__

jsion_assembler_arm_h__

@@ +54,5 @@
> +    // sometimes we want to specifically refer to the 
> +    // link register as a link register (bl lr is much
> +    // clearer than bl r14).  HOWEVER, this register can
> +    // easily be a gpr when it is not busy holding the return
> +    // address.

No need for whitespace.

@@ +187,5 @@
> +} // namespace js
> +} // namespace ion
> +
> +namespace js {
> +namespace ion {

The namespace is closed above, only to be reopened. Could we eliminate the above five lines?

@@ +250,5 @@
> +        LessThanOrEqual = JSC::ARMAssembler::LE,
> +        Overflow = JSC::ARMAssembler::VS,
> +        Signed = JSC::ARMAssembler::MI,
> +        Zero = JSC::ARMAssembler::EQ,
> +        NonZero = JSC::ARMAssembler::NE

Could we line up the '=' signs? Not particularly important; just personal preference.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +37,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +#include "CodeGenerator-arm.h"

Newline after end of license block.

@@ +51,5 @@
> +#include "ion/IonCompartment.h"
> +
> +using namespace js;
> +using namespace js::ion;
> +// shared

Newline before comment.

::: js/src/ion/arm/CodeGenerator-arm.h
@@ +46,5 @@
> +#include "ion/shared/CodeGenerator-shared.h"
> +
> +namespace js {
> +namespace ion {
> +class OutOfLineBailout;

Newline before class.

::: js/src/ion/arm/IonFrames-arm.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Could the file be called "IonFrame-arm.h"?

@@ +36,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Missing header guard!

@@ +37,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +namespace js {

Newline before namespace.

@@ +39,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +namespace js {
> +namespace ion {
> +// Layout of the frame prefix. This assumes the stack architecture grows down.

Newline before //.

@@ +47,5 @@
> +  protected:
> +    void *returnAddress_;
> +    uintptr_t sizeDescriptor_;
> +    void *calleeToken_;
> +    void *padding;

This seems like the right place to insert a comment explaining that the padding is for some alignment guarantees. It would be even better if it could refer to a some const value that will later appear in asserts.

@@ +51,5 @@
> +    void *padding;
> +};
> +
> +}
> +}

// namespace ion
// namespace js

::: js/src/ion/arm/IonFrames-arm_flymake.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

This is the same file as IonFrames-arm.h. I'm assuming it was included erroneously.

::: js/src/ion/arm/Lowering-arm-inl.h
@@ +96,5 @@
> +    return true;
> +}
> +
> +} // namespace js
> +} // namespace ion

These are backwards.

::: js/src/ion/arm/Lowering-arm.h
@@ +98,5 @@
> +
> +typedef LIRGeneratorARM LIRGeneratorSpecific;
> +
> +} // namespace js
> +} // namespace ion

Also backwards.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +46,5 @@
> +
> +namespace js {
> +namespace ion {
> +
> +    class MacroAssemblerARM : public Assembler

Erroneous whitespace before 'class'.
Comment 3 Marty Rosenberg [:mjrosenb] 2011-08-31 16:56:43 PDT
Created attachment 557371 [details] [diff] [review]
now with fixes

fwiw, all of the inverted js/ion comments were copied directly from other code.
I can fix all of them, but it is kind of alot.
~/src/ionmonkey/ionmonkey-683300/js/src/ion; grep -rn '// namespace js' -A1 . | grep 'namespace ion' -B1
./arm/Architecture-arm.h~:146:} // namespace js
./arm/Architecture-arm.h~-147-} // namespace ion
--
./shared/Lowering-shared-inl.h:245:} // namespace js
./shared/Lowering-shared-inl.h-246-} // namespace ion
--
./IonAnalysis.h:73:} // namespace js
./IonAnalysis.h-74-} // namespace ion
--
./x86/Lowering-x86.h:97:} // namespace js
./x86/Lowering-x86.h-98-} // namespace ion
--
./x86/Lowering-x86-inl.h:99:} // namespace js
./x86/Lowering-x86-inl.h-100-} // namespace ion
--
./x86/Architecture-x86.h:136:} // namespace js
./x86/Architecture-x86.h-137-} // namespace ion
--
./x86/Assembler-x86.h:161:} // namespace js
./x86/Assembler-x86.h-162-} // namespace ion
--
./IonRegisters.h:354:} // namespace js
./IonRegisters.h-355-} // namespace ion
--
./x64/Assembler-x64.h:169:} // namespace js
./x64/Assembler-x64.h-170-} // namespace ion
--
./x64/Assembler-x64.h:446:} // namespace js
./x64/Assembler-x64.h-447-} // namespace ion
--
./x64/Lowering-x64.h:82:} // namespace js
./x64/Lowering-x64.h-83-} // namespace ion
--
./x64/Architecture-x64.h:162:} // namespace js
./x64/Architecture-x64.h-163-} // namespace ion
--
./IonCompartment.h:172:} // namespace js
./IonCompartment.h-173-} // namespace ion
--
./LICM.h:115:} // namespace js
./LICM.h-116-} // namespace ion
--
./Lowering.h:103:} // namespace js
./Lowering.h-104-} // namespace ion


I'm in the process of building right now.  If all goes well, it should get pushed.
Comment 4 Marty Rosenberg [:mjrosenb] 2011-08-31 17:49:53 PDT
Created attachment 557385 [details] [diff] [review]
patch, now with misc non-arm comment fixes

and this is why I'd closed the namespace then immediately re-opened it:
} // namespace js                             
} // namespace ion                            

#include "ion/shared/Assembler-x86-shared.h"

namespace js {
namespace ion {
Comment 5 Sean Stangl [:sstangl] 2011-08-31 18:04:40 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/27e9b32b8f33
Comment 6 Sean Stangl [:sstangl] 2011-08-31 18:11:25 PDT
Looks like some temporary (*~) files snuck their way into that patch.

http://hg.mozilla.org/projects/ionmonkey/rev/a9f2747976ea

Note You need to log in before you can comment on or make changes to this bug.