Bug 152503 – [JSC] Get the JavaScriptCore framework to build on ARM64 with B3 enabled

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Get the JavaScriptCore framework to build on ARM64 with B3 enabled

[Benjamin Poulain]

Created attachment 267786 [details]
Patch

[Benjamin Poulain]

Next: Div, Mod, testing, and some codegen is shit.

[Filip Pizlo]

Comment on attachment 267786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267786&action=review

Is the plan to avoid using the scratch registers on arm right from the start?

Anyway this is the right direction. If the X86Registers change is easy then it would be cool to do it in this patch. Otherwise go ahead and land anyway, I can try to clean that up during my shift.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:689
> +#if CPU(X86)

It would be nice to avoid #if's in the instruction selector. Usually we say "if (isX86())".  It appears that the only reason for #if is the fact that you need it to say "ecx". How about making X86Assembler declare X86Registers even when !CPU(X86)?  That would be a very useful change because it would let us use normal control flow in more places.

[Filip Pizlo]

I guess you can't put X86Registers outside the #if since that won't make it the canonical RegisterID type. 

I still think we should avoid #if's. They don't scale. We could do that here by having static methods in Tmp for all of the platform registers we need to access, like ecx, eax, and edx on x86 and zr and lr on arm. You don't have to do it in this patch, but what do you think of this direction?

[Benjamin Poulain]

(In reply to comment #4)
> I guess you can't put X86Registers outside the #if since that won't make it
> the canonical RegisterID type. 
> 
> I still think we should avoid #if's. They don't scale. We could do that here
> by having static methods in Tmp for all of the platform registers we need to
> access, like ecx, eax, and edx on x86 and zr and lr on arm. You don't have
> to do it in this patch, but what do you think of this direction?

I am very annoyed with those #ifdef myself. :(

I don't have a problem with the static Tmp idea but I am not sure that will lead to a better design.

If we want to be as good as LLVM, we'll have to support all the platform specific goodness available. Having platform specific lowering helper seems like a way to do that. For Div and Shift, it would work quite well to have separate functions with a different implementation for ARM64 and x86. I am not sure about more complicated cases like the full Lea.

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > I guess you can't put X86Registers outside the #if since that won't make it
> > the canonical RegisterID type. 
> > 
> > I still think we should avoid #if's. They don't scale. We could do that here
> > by having static methods in Tmp for all of the platform registers we need to
> > access, like ecx, eax, and edx on x86 and zr and lr on arm. You don't have
> > to do it in this patch, but what do you think of this direction?
> 
> I am very annoyed with those #ifdef myself. :(
> 
> I don't have a problem with the static Tmp idea but I am not sure that will
> lead to a better design.
> 
> If we want to be as good as LLVM, we'll have to support all the platform
> specific goodness available. Having platform specific lowering helper seems
> like a way to do that. For Div and Shift, it would work quite well to have
> separate functions with a different implementation for ARM64 and x86. I am
> not sure about more complicated cases like the full Lea.

I'm all for platform-specific lowering.  I just want it to be guarded with "if" and not "#if".  The only reason why we can't do it is X86Registers.  In all other regards, we're already in good shape: Air already declares all opcodes on all architectures, so X86Div is visible on ARM - it just always returns false for isValidForm, and the instruction selector can avoid using it by checking if isX86().

For Lea, it's not actually a problem of Lea but a problem of Index.  ARM has a Lea instruction, they just call it "add".  It supports all of the forms of a load instruction.  That means that it can do what we call Addr, though with a smaller available offset, and it can do what we call Index, though with the offset constrained to zero.  I believe we want to teach the Air validater that Arg::imm, Arg::addr, and Arg::index may have target-specific constraints, and the validater can reject Arg's that don't obey those constraints.  We could take this further:

- Arg can have an isValidForm() method that usually returns true, but will return false if Arg::imm is out of range for the target's immediate format, or if Arg::addr has an offset that is out of range for the address offset immediate on the target, or if Arg::index has a non-zero offset on ARM.

- Inst::isValidForm() could be made to call Arg::isValidForm() on all of its arguments.

- The standalone isValidForm() will not be able to validate the Arg, since it only gets a Arg::Kind, but the instruction selector could know to call Arg::isValidForm() on its imms and addrs.

As for registers, here are two idioms for X86Registers that might work:

- Tmp::x86EAX(), Tmp::x86ECX(), and Tmp::x86EDX() as a way of getting those registers.

- Add a namespace called x86 to Air, so we can say Air::x86::eax() or just x86::eax().

In either naming scheme, you get the registers using a method call so that the method can UNREACHABLE_FOR_PLATFORM() if !x86.

[Filip Pizlo]

But anyway, LGTM to land.  Better get it in the tree, and then we can figure it out.  It's not the end of the world if we live with these #ifs for a short while.

[WebKit Commit Bot]

Comment on attachment 267786 [details]
Patch

Clearing flags on attachment: 267786

Committed r194388: <http://trac.webkit.org/changeset/194388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This seems to have broken tests on Windows. Can one of you please take a look?

[Benjamin Poulain]

(In reply to comment #10)
> This seems to have broken tests on Windows. Can one of you please take a
> look?

That code does not run on Windows. Can you give more detail?