Closed Bug 710551 Opened 12 years ago Closed 12 years ago

IonMonkey: Finish implementing visitCallGeneric

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mjrosenb, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

I like to spread the blame, so I'm blaming sstangl for finding this bug, and myself for introducing the bug.  As it turns out, the last few lines of visitCallGeneric have been
#if 0
x86_specific_code
#endif
return false;
meaning we never actually compiled a call :-(
I assume that the new code in spillRegs needs to be re-written, but this works, and the old code didn't
Attachment #581546 - Attachment is obsolete: true
Attachment #581558 - Flags: review?(sstangl)
Attachment #581558 - Attachment is obsolete: true
Attachment #581558 - Flags: review?(sstangl)
Attachment #581831 - Flags: review?(sstangl)
Comment on attachment 581831 [details] [diff] [review]
Also fixes some hilarity with snapshots + temp registers

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

r+ with comments addressed.

::: js/src/ion/LIR-Common.h
@@ +253,5 @@
>      }
>      const LAllocation *getNargsReg() {
>          return getTemp(1)->output();
>      }
> +    const LAllocation *getObject() {

getObject() is easily confusable with getFunction(). Could we call it getObjectTemp()?

::: js/src/ion/LIR.h
@@ +798,1 @@
>              FloatRegisterSet::All()

nit: additional (incorrect) indentation added.

::: js/src/ion/Lowering.cpp
@@ +142,5 @@
>      // A call is entirely stateful, depending upon arguments already being
>      // stored in an argument vector. Therefore visitCall() may be generic.
>      LCallGeneric *ins = new LCallGeneric(useRegister(call->getFunction()),
>                                           argslot, temp(LDefinition::POINTER),
> +                                         temp(LDefinition::POINTER), temp(LDefinition::POINTER));

nit: last temp() on a newline, please.

::: js/src/ion/arm/Assembler-arm.h
@@ +106,5 @@
>  static const FloatRegister d14 = {FloatRegisters::d14};
>  static const FloatRegister d15 = {FloatRegisters::d15};
>  
> +// For maximal awesomeness, 8 should be sufficent.
> +static const uint32 StackAlignment = 8;

nit: A comment explaining why this number was chosen / why it is needed at all would be nice.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +118,5 @@
>      // Note that this automatically sets MacroAssembler::framePushed().
>      masm.reserveStack(frameSize());
> +#ifdef DEBUG
> +    masm.checkStackAlignment();
> +#endif

Instead of having "#ifdef DEBUG" every time this function is called, can we make the function inline, and surround the body with "#ifdef DEBUG"? Renaming the function to debugCheckStackAlignment() would then make it clear that it only executes in debug mode.

@@ +1098,5 @@
>      uint32 argslot = arg->argslot();
>      int32 stack_offset = StackOffsetOfPassedArg(argslot);
>  
> +    masm.ma_str(val.typeReg(), DTRAddr(StackPointer, DtrOffImm(stack_offset+4)));
> +    masm.ma_str(val.payloadReg(), DTRAddr(StackPointer, DtrOffImm(stack_offset)));

visitStackArg() should be moved to CodeGenerator.{cpp,h}. The above logic can be expressed commonly by implementing storeValue() in the ARM MacroAssembler. The x86/x64 versions are already identical.

@@ +1129,5 @@
>      Register nargsreg = ToRegister(nargs);
>  
>      uint32 callargslot  = call->argslot();
>      uint32 unused_stack = StackOffsetOfPassedArg(callargslot);
> +    JS_ASSERT((unused_stack & 0x7) == 0);

This is not necessarily valid, as far as I know, and I think its validity is not necessary for correct operation. We just need the base of /something/ to be 8-byte aligned -- I think the base of the StackFrame after the return value is pushed (is it pushed?), which is independent of the unused_stack.

@@ +1227,2 @@
>      // Increment to remove IonFramePrefix; decrement to fill FrameSizeClass.
> +    int prefix_garbage = 3 * sizeof(void *);

This "3" scares me more than the "2" did. As you mentioned on IRC, this really should be a sizeof().

@@ +1233,2 @@
>      else if (restore_diff < 0)
> +        masm.ma_sub(Imm32(-restore_diff), StackPointer);

These really should be addPtr/subPtr. If we can express code on ARM in the same way as x86/x64, using similar MacroAssembler names, then it becomes easier to notice when code may be merged. In this case, I think visitCallGeneric() is already fairly close to being platform-independent, which would be *great*.

@@ +1238,1 @@
>      return false;

nit: -remove false;

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +565,5 @@
>      // ARM's ABI does not ask you to put the return address on the stack, so we don't
>      // We still need to have the return address, so we use the pc to compute it.
>      // Presently, I'm hard coding the value because I know exactly what code is being generated
>      // but I'll set up a as_mov(Register, Label *) that will automatically do this.
> +    masm.breakpoint();

Trace/breakpoint trap. (Core dumped)

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +128,5 @@
>          JS_ASSERT(slot >= 0 && slot <= int32(graph.argumentSlotCount()));
>          int32 offset = masm.framePushed() -
>                         (graph.localSlotCount() * STACK_SLOT_SIZE) -
>                         (slot * sizeof(Value));
> +        offset &= ~7;

Please include a comment on why shifting down is safe: there is nothing beneath the passed argument slots, and the passed argument slots are Value-sized. (This logic would be incorrect if we used the other definition of "slots" here, and so it's ripe for confusion.)
Attachment #581831 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/806416b0a628
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.