Closed
Bug 1483030
Opened 6 years ago
Closed 6 years ago
Implement many functions for basic ARM64 codegen
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: sstangl, Assigned: sstangl)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•6 years ago
|
||
Sorry, just got around to seeing this after blackholing into some compression work. Reviewing now.
Comment 2•6 years ago
|
||
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+
Assignee | ||
Comment 3•6 years ago
|
||
(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!
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d81eb0bdb46a
Implement basic ARM64 codegen. r=djvj
Keywords: checkin-needed
Comment 5•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•