Last Comment Bug 710551 - IonMonkey: Finish implementing visitCallGeneric
: IonMonkey: Finish implementing visitCallGeneric
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-12-13 22:43 PST by Marty Rosenberg [:mjrosenb]
Modified: 2011-12-22 11:48 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP, some basic tests work, others still fail. (10.92 KB, patch)
2011-12-13 22:43 PST, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
probably still a WIP, but all failures I've seen are from a lack of features (11.83 KB, patch)
2011-12-14 01:01 PST, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
Also fixes some hilarity with snapshots + temp registers (15.74 KB, patch)
2011-12-14 16:45 PST, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2011-12-13 22:43:19 PST
Created attachment 581546 [details] [diff] [review]
WIP, some basic tests work, others still fail.

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 :-(
Comment 1 Marty Rosenberg [:mjrosenb] 2011-12-14 01:01:54 PST
Created attachment 581558 [details] [diff] [review]
probably still a WIP, but all failures I've seen are from a lack of features

I assume that the new code in spillRegs needs to be re-written, but this works, and the old code didn't
Comment 2 Marty Rosenberg [:mjrosenb] 2011-12-14 16:45:46 PST
Created attachment 581831 [details] [diff] [review]
Also fixes some hilarity with snapshots + temp registers
Comment 3 Sean Stangl [:sstangl] 2011-12-14 17:31:32 PST
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.)
Comment 4 David Anderson [:dvander] 2011-12-22 11:48:36 PST
http://hg.mozilla.org/projects/ionmonkey/rev/806416b0a628

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