Closed Bug 664591 Opened 13 years ago Closed 13 years ago

IonMonkey: Implement code generation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(4 files, 3 obsolete files)

It's about time for IonMonkey to be able to run some stuff.
Attached patch rough sketch (obsolete) — Splinter Review
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.
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.
Attached patch reorganized (obsolete) — Splinter Review
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
Attached patch clean up registers (obsolete) — Splinter Review
Re-use more JSC assembler definitions, rename verbose [Float]RegisterCodes to [Float]Registers.
Attachment #544722 - Attachment is obsolete: true
Attached patch wip v0Splinter Review
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
Attached patch more opsSplinter Review
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 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 on attachment 545105 [details] [diff] [review]
more ops

Review of attachment 545105 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #545105 - Flags: review?(adrake) → review+
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+
LArguments are just completely wrong now, this fixes their offset.
Attachment #545288 - Flags: review?(adrake)
Comment on attachment 545288 [details] [diff] [review]
part 4: fix LArguments

Review of attachment 545288 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #545288 - Flags: review?(adrake) → review+
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.
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.