Last Comment Bug 701125 - IonMonkey: moveValue needs a relocation marker
: IonMonkey: moveValue needs a relocation marker
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: IonMonkey
  Show dependency treegraph
Reported: 2011-11-09 11:59 PST by Jan de Mooij [:jandem]
Modified: 2011-11-22 10:08 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (9.32 KB, patch)
2011-11-11 00:18 PST, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2011-11-09 11:59:06 PST
Both x86 and x64. x86 handles this correctly in visitValue but not in moveValue. We still need to fix moveValue (and probably just call moveValue from visitValue, like we do on x64).
Comment 1 User image David Anderson [:dvander] 2011-11-11 00:18:41 PST
Created attachment 573760 [details] [diff] [review]

Great catch. x86 fix is a little gross - I could make it look like the x64 one as well.
Comment 2 User image Jan de Mooij [:jandem] 2011-11-11 01:50:46 PST
Comment on attachment 573760 [details] [diff] [review]

Review of attachment 573760 [details] [diff] [review]:

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +131,5 @@
>          movq(op, dest);
>      }
>      void moveValue(const Value &val, const Register &dest) {
>          movq(ImmWord((void *)val.asRawBits()), dest);
> +        if (val.isGCThing())

The x86 implementation in visitValue calls isMarkable instead of isGCThing. Do we need isGCThing?

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +107,5 @@
> +            gc::Cell *cell = reinterpret_cast<gc::Cell *>(val.toGCThing());
> +            movl(ImmGCPtr(cell), data);
> +        } else {
> +            movl(Imm32(jv.s.payload.i32), data);
> +        }

Can we call this function from CodeGeneratorX86::visitValue?

I agree that rewriting this to be more like the x64 version may be nice, but it doesn't matter too much so I will leave that up to you.
Comment 3 User image David Anderson [:dvander] 2011-11-22 10:08:07 PST (with a follow-up fix for nits)

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