Closed Bug 1127269 Opened 5 years ago Closed 3 years ago

More ARM cleanup

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Part 1 - Assembler-arm.h (obsolete) — Splinter Review
This first patch fixes a number of issues I found by going through Assembler-arm.h:

* Made most single-arg constructors |explicit|
* A lot of classes now have private members
* Added some asserts when storing values in bitfields, to make sure we don't lose information
* A ton of style nits

I didn't make the Operand2(Address) constructor explicit yet, because that blows up. Will look into that separately.
Attachment #8556404 - Flags: review?(dtc-moz)
Comment on attachment 8556404 [details] [diff] [review]
Part 1 - Assembler-arm.h

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

Looks good, thank you.

::: js/src/jit/arm/Assembler-arm.h
@@ +729,5 @@
>      uint32_t data;
>  
>    protected:
>      DtrOff(datastore::Imm12Data immdata, IsUp_ iu)
> +      : data(immdata.encode() | uint32_t(IsImmDTR) | (uint32_t(iu)))

"(uint32_t(iu))" to "uint32_t(iu)" ?
Attachment #8556404 - Flags: review?(dtc-moz) → review+
The patch does not apply anymore.
Flags: needinfo?(jdemooij)
Priority: -- → P5
Attached patch Part 1 - Assembler-arm.h (obsolete) — Splinter Review
Some of these changes were done in other bugs in the meantime. Here's a new patch for the remaining things.
Attachment #8556404 - Attachment is obsolete: true
Flags: needinfo?(jdemooij)
Attachment #8803867 - Flags: review?(sstangl)
Forgot to qref.
Attachment #8803867 - Attachment is obsolete: true
Attachment #8803867 - Flags: review?(sstangl)
Attachment #8803868 - Flags: review?(sstangl)
Comment on attachment 8803868 [details] [diff] [review]
Part 1 - Assembler-arm.h

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

LGTM. Thank you for doing this.
Attachment #8803868 - Flags: review?(sstangl) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10205a882c3c
Clean up some code in the ARM backend. r=sstangl
https://hg.mozilla.org/mozilla-central/rev/10205a882c3c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.