Closed
Bug 664591
Opened 13 years ago
Closed 13 years ago
IonMonkey: Implement code generation
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(4 files, 3 obsolete files)
90.06 KB,
patch
|
adrake
:
review+
|
Details | Diff | Splinter Review |
26.52 KB,
patch
|
adrake
:
review+
|
Details | Diff | Splinter Review |
10.80 KB,
patch
|
adrake
:
review+
|
Details | Diff | Splinter Review |
3.47 KB,
patch
|
adrake
:
review+
|
Details | Diff | Splinter Review |
It's about time for IonMonkey to be able to run some stuff.
Assignee | ||
Comment 1•13 years ago
|
||
Starting this now. This is a super rough sketch, most of the pieces are going to be moved around. Code generation will be shared at a few levels. * Writing snapshots, ICs, etc will be shared at the topmost level. * Similar platforms share from there. x86, x64 have a shared codegen, in theory ARM and SPARC could share a 3-reg code generator. * CPU-specific derives again. So x86, x64 both have a code generator that derives from x86-shared.
Assignee | ||
Comment 2•13 years ago
|
||
The design space for the assembler is pretty large. IonMonkey really needs two level of abstractions, oen for ICs, and one for normal LIR. ICs want an interface like: void generate(RegisterID reg1, RegisterID reg2) { masm.load(Address(reg1, offset), reg2); } Whereas LIR wants an interface like: void generate(const LAllocation *in, const LDefinition *out) { masm.load(ToSourceOperand(in), ToDestOperand(out)); } The former example knows the encoding ahead of time, whereas the latter does not. We could get away with re-using MacroAssembler*.h, and adding stuff like: struct Operand { union { Address disp; BaseIndex sib; RegisterID reg; }; Kind kind; }; And then adding the appropriate functions to MacroAssembler*.h. This patch leans in that direction, but rather than using MacroAssembler*.h, replaces it with a new abstraction layer and implements it using X86Assembler, ARMAssembler, etc. This is appealing to me because we have the chance to opt into data structures that really fit our uses and future concerns (like relocation, more robust patching and memory management). We also need a stronger separation between implicit and explicit behavior. For example, the MacroAssembler often has to insert multiple instructions and reserve registers to abstract common operation X across all platforms. We don't want that to bite us. So, this patch is a super rough sketch, some things are clearly non-starters and the organization is poor. I'll iterate a few more times tomorrow.
Assignee | ||
Comment 3•13 years ago
|
||
I like this organization a little better. Now we have: shared/CodeGenerator-shared: shared between x86, x64, ARM shared/CodeGenerator-x86-shared: shared between x86, x64 x86/CodeGenerator-x86 x64/CodeGenerator-x64 For the assembler: shared/Assembler-x86-shared: shared between x86, x64 x86/Assembler-x86 x64/Assembler-x64 The assemblers will all be platform specific. I.e., an Operand() on x86, x64, and ARM are different. Once we need an abstraction layer across all platforms, we can start building a separate ion::MacroAssembler that looks more like JSC::MacroAssembler, potentially implementing one abstract assembly operation as multiple commands in the ISA-specific assembler. Next up, figuring out what to do with IonRegisters.h.
Attachment #544187 -
Attachment is obsolete: true
Assignee | ||
Comment 4•13 years ago
|
||
Re-use more JSC assembler definitions, rename verbose [Float]RegisterCodes to [Float]Registers.
Attachment #544722 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
Can now compile, but not run, my test function from http://xkcd.com/221/ [jaeger] Insns movabsq $0xfff8800000000004, %rcx [jaeger] Insns pop %rbp [jaeger] Insns ret
Attachment #544923 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #544945 -
Flags: review?(adrake)
Assignee | ||
Comment 6•13 years ago
|
||
This patch applies on top of the earlier one and implements Unbox/UnboxInteger, Box, prologues, stack use, and AddI. It leaves off doubles and deoptimizations, which will come later (probably after LMoves).
Attachment #545105 -
Flags: review?(adrake)
Comment 7•13 years ago
|
||
Comment on attachment 544945 [details] [diff] [review] wip v0 Review of attachment 544945 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/GreedyAllocator.h @@ +69,5 @@ > LDefinition *def; > uint32 stackSlot_; > union { > + Registers::Code gprCode; > + FloatRegisters::Code fpuCode; Seeing a plural class name like that in the context of a type seems kind of jarring to the reader to me, thoughts? ::: js/src/ion/IonLowering.cpp @@ +151,5 @@ > MInstruction *opd = test->getInput(0); > MBasicBlock *ifTrue = test->ifTrue(); > MBasicBlock *ifFalse = test->ifFalse(); > > + if (opd->isConstant()) { Shouldn't this happen during dead code elimination? It seems a bit odd to do this kind of heavy-handed optimization in the code generator. ::: js/src/ion/shared/Assembler-shared.h @@ +90,5 @@ > + int32 offset_ : 31; > + bool bound_ : 1; > + > + // Disallow assignment. > + void operator =(const Label &label) { } I wouldn't even bother defining this -- it shouldn't make it to the linker anyway, and if it does it caught another bug.
Attachment #544945 -
Flags: review?(adrake) → review+
Comment 8•13 years ago
|
||
Comment on attachment 545105 [details] [diff] [review] more ops Review of attachment 545105 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #545105 -
Flags: review?(adrake) → review+
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #545270 -
Flags: review?(adrake)
Comment 10•13 years ago
|
||
Comment on attachment 545270 [details] [diff] [review] part 3: generate for LMove Review of attachment 545270 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/x86/CodeGenerator-x86.cpp @@ +57,5 @@ > return true; > } > > bool > +CodeGenerator::visitMove(LMove *move) There are exactly two characters of difference between this and the x64 version, just an observation.
Attachment #545270 -
Flags: review?(adrake) → review+
Assignee | ||
Comment 11•13 years ago
|
||
LArguments are just completely wrong now, this fixes their offset.
Attachment #545288 -
Flags: review?(adrake)
Comment 12•13 years ago
|
||
Comment on attachment 545288 [details] [diff] [review] part 4: fix LArguments Review of attachment 545288 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #545288 -
Flags: review?(adrake) → review+
Assignee | ||
Comment 13•13 years ago
|
||
I have pushed these patches so I'm no longer blocking other work, but there are still two big pieces to go: (1) LMoves right now do not correctly handle memory to memory moves. (2) JIT code gets generated but not allocated or attached to anything. But, we generate *stuff*, and it's possible to see it with: JMFLAGS=insns IONFLAGS=whatever path/to/js ... I'll file a few follow-up bugs momentarily on the necessary next steps to make codegen useful.
Assignee | ||
Comment 14•13 years ago
|
||
bug 670816, bug 670819, bug, 670820, bug 670822 filed as immediate todos to get code running.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•