Closed
Bug 683300
Opened 13 years ago
Closed 13 years ago
IonMonkey skeleton for ARM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file, 3 obsolete files)
192.57 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
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.
Attachment #556978 -
Attachment is obsolete: true
Attachment #557067 -
Flags: review?(sstangl)
Comment 2•13 years ago
|
||
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'.
Attachment #557067 -
Flags: review?(sstangl) → review+
Reporter | ||
Comment 3•13 years ago
|
||
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.
Attachment #557067 -
Attachment is obsolete: true
Reporter | ||
Comment 4•13 years ago
|
||
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 {
Attachment #557371 -
Attachment is obsolete: true
Comment 5•13 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/27e9b32b8f33
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 6•13 years ago
|
||
Looks like some temporary (*~) files snuck their way into that patch. http://hg.mozilla.org/projects/ionmonkey/rev/a9f2747976ea
You need to log in
before you can comment on or make changes to this bug.
Description
•