Closed Bug 1415772 Opened 7 years ago Closed 7 years ago

IonMonkey: Implement MNearbyInt recover instruction

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jthemphill, Assigned: jthemphill, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171108110838

Steps to reproduce:

Implement MNearbyInt(RoundingMode::Up) and MNearbyInt(RoundingMode::Down) in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

This issue is similar to Bug 1104647, but for processors which support SSE4.1 instructions.

When asked to floor or ceil a double (not an int, object, or JavaScript "number") into a double, MCallOptimize.cpp checks if the processor supports SSE4.1 instructions. If it does, it delegates to MNearbyInt. If it doesn't, it delegates to MathFunction(Floor) or MathFunction(Ceil).
Blocks: 1003801
Mentor: nicolas.b.pierron
Priority: -- → P5
Severity: normal → enhancement
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Attachment #8926772 - Flags: review?(nicolas.b.pierron)
Assignee: nobody → jthemphill
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8926772 [details]
Bug 1415772: Implement MNearbyInt recover instruction

https://reviewboard.mozilla.org/r/198010/#review203342

This patch looks good, still a few issues before being mergeable.

::: js/src/jit/MIR.h:12628
(Diff revision 1)
>  
>      void printOpcode(GenericPrinter& out) const override;
>  
> +    MOZ_MUST_USE bool writeRecoverData(CompactBufferWriter& writer) const override;
> +
> +    bool canRecoverOnBailout() const override { return true; }

This is only true for the rounding mode up and down.

::: js/src/jit/Recover.cpp:992
(Diff revision 1)
> +bool
>  MMathFunction::writeRecoverData(CompactBufferWriter& writer) const
>  {
>      MOZ_ASSERT(canRecoverOnBailout());
>      switch (function_) {
> -      case Floor:
> +        case Floor:

nit: add Ceil for --no-see4 as well?

::: js/src/jit/Recover.cpp:993
(Diff revision 1)
>  MMathFunction::writeRecoverData(CompactBufferWriter& writer) const
>  {
>      MOZ_ASSERT(canRecoverOnBailout());
>      switch (function_) {
> -      case Floor:
> +        case Floor:
> -        writer.writeUnsigned(uint32_t(RInstruction::Recover_Floor));
> +            writer.writeUnsigned(uint32_t(RInstruction::Recover_Floor));

nit: SpiderMonkey coding style is quite special, and uses half-indent for case statements.
Attachment #8926772 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8926772 [details]
Bug 1415772: Implement MNearbyInt recover instruction

https://reviewboard.mozilla.org/r/198010/#review204124

Perfect!
Attachment #8926772 - Flags: review?(nicolas.b.pierron) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc99e9783120
Implement MNearbyInt recover instruction r=nbp
https://hg.mozilla.org/mozilla-central/rev/bc99e9783120
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: