Closed
Bug 716469
Opened 13 years ago
Closed 13 years ago
IonMonkey: Port ICs to ARM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjrosenb, Unassigned)
Details
Attachments
(1 file, 2 obsolete files)
18.78 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Bug 707854 added ICs for JSOP_GETPROP to IonMonkey, but only implemented the backend for x86 and x64. For the most part, the port to ARM is straightforward. The biggest hitch is IonMonkey's new dynamically allocated pools mean that while assembling code, we don't actually know what the offset of anything is supposed to be until linking is done.
I'm currently in the process of getting the last few bugs out of the patch.
Reporter | ||
Comment 1•13 years ago
|
||
Attachment #586923 -
Attachment is obsolete: true
Attachment #587930 -
Flags: review?(dvander)
Comment on attachment 587930 [details] [diff] [review]
implement ICs on ARM
Review of attachment 587930 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with some refactoring:
::: js/src/ion/Ion.cpp
@@ +501,4 @@
> {
> memcpy(cacheList(), caches, numCaches() * sizeof(IonCache));
>
> + masm.resetCounter();
Can we rename this to "beginOffsetComputation" or something?
::: js/src/ion/IonCaches.cpp
@@ +170,5 @@
> +IonCache::updateBaseAddress(IonCode *code, MacroAssembler &masm)
> +{
> + initialJump_.repoint(code, masm.additionalOffset(initialJump_.offset()));
> + lastJump_.repoint(code, masm.additionalOffset(lastJump_.offset()));
> + cacheLabel_.repoint(code, masm.additionalOffset(cacheLabel_.offset()));
If possible, I would try to rearrange this so repoint looks like:
void
CodeLocationJump::repoint(IonCode *code, MacroAssembler &masm)
And then change additionalOffset to be:
ptrdiff_t
AssemblerXYZ::actualOffsetOf(size_t offset) ...
Bonus points if actualOffsetOf takes in a CodeLocationJump instead.
::: js/src/ion/IonMacroAssembler.h
@@ +173,1 @@
> return CodeOffsetLabel(size());
This function should be moved into MacroAssembler-$(ARCH) to avoid the #ifdef
::: js/src/ion/shared/Assembler-x86-shared.h
@@ +40,5 @@
> * ***** END LICENSE BLOCK ***** */
>
> #ifndef jsion_assembler_x86_shared__
> #define jsion_assembler_x86_shared__
> +#include <cstddef>
What is this for?
::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +713,5 @@
> + void resetCounter() {
> + curDumpsite = 0;
> + }
> + ptrdiff_t poolSizeBefore(ptrdiff_t offset) {
> + while(curDumpsite < numDumps && poolInfo[curDumpsite].offset <= offset)
Space in between while and (
Attachment #587930 -
Flags: review?(dvander) → review+
Comment on attachment 587930 [details] [diff] [review]
implement ICs on ARM
Review of attachment 587930 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/IonMacroAssembler.h
@@ +166,5 @@
> storeCallResult(dest.typedReg());
> }
>
> CodeOffsetLabel labelForPatch() {
> +#if defined(JS_CPU_ARM)
Errr, this is the #ifdef we should remove, in labelForPatch -- splinter did me no favors
Reporter | ||
Comment 4•13 years ago
|
||
Fixing some of the nits required a bit of code refactoring, so I'm posting a new patch. You probably only want to look at the differences between the two patches.
Attachment #587930 -
Attachment is obsolete: true
Attachment #590775 -
Flags: review?(dvander)
Comment on attachment 590775 [details] [diff] [review]
With a bit more
Review of attachment 590775 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/IonMacroAssembler.h
@@ +194,5 @@
> }
>
> CodeOffsetLabel labelForPatch() {
> +#if defined(JS_CPU_ARM)
> + return CodeOffsetLabel(nextOffset().getOffset());
This should be moved into individual MacroAssembler-$(ARCH) s
::: js/src/ion/shared/Assembler-x86-shared.h
@@ +832,5 @@
> }
> +
> + void resetCounter() {}
> + ptrdiff_t actualOffset(uint8* x) {
> + return 0;
hrm!
(Also, resetCounter should be more descriptive - maybe prepareFor...Something)
Attachment #590775 -
Flags: review?(dvander) → review+
Reporter | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•