Closed
Bug 1488551
Opened 6 years ago
Closed 5 years ago
Create NumberOperandId class in CacheIR
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla71
People
(Reporter: mgaudet, Assigned: tetsuharu)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
We have enough methods in CacheIR that are manipulating Numbers that we ought to have a type for it: We could return NumberOperandId from guardIsNumber and guardAndGetNumberFromString, and then use it as the argument type to compareDoubleResult, to provide a little type safety.
Reporter | ||
Comment 1•6 years ago
|
||
Perhaps one question would be: Should Int32OperandId inherit from NumberOperandId?
Comment 2•6 years ago
|
||
(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #1) > Perhaps one question would be: Should Int32OperandId inherit from > NumberOperandId? I think NumberOperandId should inherit from ValOperandId because a number is represented as a boxed Value (because it can be double or int32). Int32OperandId OTOH has to be a TypedOperandId because it's just the payload. This matters for the register allocator.
Assignee | ||
Comment 3•5 years ago
|
||
I'll take this.
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → tetsuharu.ohzeki
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
This changes:
- Take
NumberOperandId
at the part which can use the result ofCacheIRWriter.guardIsNumber()
. - Use
NumberOperandId
as a function signature which would only take a number operand.- There are some exceptions, i.e.
CacheIRWriter.writer.truncateDoubleToUInt32()
,
becauseNumberOperandId
is not alwaysdouble
.
- There are some exceptions, i.e.
Updated•5 years ago
|
Attachment #9094290 -
Attachment is obsolete: true
Pushed by mgaudet@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a5339ab7cb7 Create NumberOperandId to make some number operations in CacheIRWriter/CacheIRCompiler more type safe. r=mgaudet
Comment 7•5 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Comment 8•5 years ago
|
||
This doesn't look right: https://searchfox.org/mozilla-central/diff/9cc0db75fed04cfada0e50be572f101cb55fd075/js/src/jit/CacheIR.h#138
Do you mind removing this (unused) constructor?
Flags: needinfo?(tetsuharu.ohzeki)
Assignee | ||
Comment 9•5 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #8)
This doesn't look right: https://searchfox.org/mozilla-central/diff/9cc0db75fed04cfada0e50be572f101cb55fd075/js/src/jit/CacheIR.h#138
Do you mind removing this (unused) constructor?
I'm sorry. I added it to sort with others.
I'll push the follow up patch and ask you to review it.
Flags: needinfo?(tetsuharu.ohzeki)
Assignee | ||
Comment 10•5 years ago
|
||
I added this constructor to sort with other types.
However, it was mistake. We don't use it.
Assignee | ||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
Pushed by tetsuharu.ohzeki@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1e0685b06c61 followup: remove unused constructor `TypedOperandId(NumberOperandId id)`. r=jandem
Comment 13•5 years ago
|
||
bugherder |
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•