Closed Bug 1483030 Opened Last year Closed Last year

Implement many functions for basic ARM64 codegen

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The attached patch implements many basic functions required for ARM64 codegen. In most cases, the logic is intended to be similar to the x64 variant.
Attachment #8999747 - Flags: review?(kvijayan)
Sorry, just got around to seeing this after blackholing into some compression work.  Reviewing now.
Comment on attachment 8999747 [details] [diff] [review]
0001-Implement-basic-ARM64-codegen.patch

Review of attachment 8999747 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Sorry for the delay.  No review fixes.  I didn't trace back through the presumably ARM-specific functions and code you were using (e.g. the register handling stuff).  Guessing you gave that to other people to review.

::: js/src/jit/arm64/CodeGenerator-arm64.cpp
@@ +420,5 @@
> +    }
> +
> +    masm.And(outw, lhsw, Operand((uint32_t(1) << shift) - 1));
> +
> +    if (canBeNegative) {

I wonder if negative modulo is so common that we need a fastpath for it?  Are you cribbing this largely from logic for the other architecture codegens?

Wouldn't be surprised if there was some octane test behind this particular codepath.. ;)

No review asks, just commenting.

::: js/src/jit/arm64/Lowering-arm64.cpp
@@ +181,4 @@
>  void
>  LIRGeneratorARM64::defineUntypedPhi(MPhi* phi, size_t lirIndex)
>  {
> +    defineTypedPhi(phi, lirIndex);

I don't have the background to understand this implementation.  Can you explain why defineUntypedPhi => defineTypedPhi, or what those ops are?  Not a review request, just curious about what the difference between the two are and why they exist?  Are there other archs where defineUntypedPhi doesn't just forward to defineTypedPhi?
Attachment #8999747 - Flags: review?(kvijayan) → review+
(In reply to Kannan Vijayan [:djvj] from comment #2)
> ::: js/src/jit/arm64/Lowering-arm64.cpp
> @@ +181,4 @@
> >  void
> >  LIRGeneratorARM64::defineUntypedPhi(MPhi* phi, size_t lirIndex)
> >  {
> > +    defineTypedPhi(phi, lirIndex);
> 
> I don't have the background to understand this implementation.  Can you
> explain why defineUntypedPhi => defineTypedPhi, or what those ops are?  Not
> a review request, just curious about what the difference between the two are
> and why they exist?  Are there other archs where defineUntypedPhi doesn't
> just forward to defineTypedPhi?

Yeah, these functions are not named well. The code is really just determining whether it will need one register to resolve this phi, or whether it will need two registers.

The function defineTypedPhi() is shared code across all architectures that defines the phi based on a single payload register. Since the type is known, it doesn't need to be explicitly carried in a register.

The function defineUntypedPhi() needs the type tag to be somewhere in a register.

On 32-bit systems, defineUntypedPhi() is implemented internally as actually being *two different* phis, one for the payload, and one for the register.

But on 64-bit systems, the payload is stored within the full Value, which consumes only a single register. Since there's only a single register, even though that register holds type information while defineTypedPhi()'s register won't, from the perspective of lowering, the only important characteristic is how many registers are used. So 64-bit architectures just call defineTypedPhi(), since they want the same behavior, and lowering doesn't assume anything about the contents of that register.

Those functions should definitely be defined in terms of helper functions with better names. This is a completely useless implementation detail that no one should have to remember. I'll file a bug, thanks for pointing it out!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d81eb0bdb46a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.