Closed Bug 1488551 Opened 6 years ago Closed 5 years ago

Create NumberOperandId class in CacheIR

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox63 --- wontfix
firefox71 --- fixed

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.
Perhaps one question would be: Should Int32OperandId inherit from NumberOperandId?
(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.

I'll take this.

Assignee: nobody → tetsuharu.ohzeki
Status: NEW → ASSIGNED
Blocks: 1582638
No longer blocks: 1582638

This changes:

  1. Take NumberOperandId at the part which can use the result of CacheIRWriter.guardIsNumber().
  2. Use NumberOperandId as a function signature which would only take a number operand.
    • There are some exceptions, i.e.CacheIRWriter.writer.truncateDoubleToUInt32(),
      because NumberOperandId is not always double.
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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)

(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)

I added this constructor to sort with other types.
However, it was mistake. We don't use it.

Pushed by tetsuharu.ohzeki@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1e0685b06c61
followup: remove unused constructor `TypedOperandId(NumberOperandId id)`. r=jandem
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: