Closed Bug 1028629 Opened 8 years ago Closed 8 years ago

Remove a function pointer (encodeVFP)

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file, 1 obsolete file)

The encodeVFP function pointer confuses the rooting hazard analysis.
This makes the hazard analysis happier, and doesn't seem to be any less clear. Gives the optimizer a tiny bit less work to do, too. :-)
Attachment #8444026 - Flags: review?(mrosenberg)
Comment on attachment 8444026 [details] [diff] [review]
Remove a function pointer (encodeVFP)

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

::: js/src/jit/arm/Assembler-arm.cpp
@@ +2108,1 @@
>      }

I think you wanted to call VN/VM?
Attachment #8444026 - Flags: review?(mrosenberg)
And it still compiles! Sorry about that.
Attachment #8444508 - Flags: review?(mrosenberg)
Attachment #8444026 - Attachment is obsolete: true
Blocks: 1022773
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)

r=mjrosenb via IRC
Attachment #8444508 - Flags: review?(mrosenberg) → review+
https://hg.mozilla.org/mozilla-central/rev/4d9344e41a56
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling the hazard analysis on b2g, bug 898554
User impact if declined: none
Testing completed: it's been in mozilla-central for a month
Risk to taking this patch (and alternatives if risky): very very low
String or UUID changes made by this patch: none

Approval Request Comment
[Feature/regressing bug #]: b2g hazard analysis, bug 898554
[User impact if declined]: none
[Describe test coverage new/current, TBPL]: mozilla-central for a month
[Risks and why]: very very low. See below.
[String/UUID change made/needed]: none

This is really a hygiene thing. Right now, aurora and b2g32 both report 2 rooting hazards, and I have the threshold set to 4 (so the job will turn red if more than 4 hazards are detected.) The 2 hazards are false positives that this patch would fix. There is no reason not to lower the threshold to 2, so that any added hazard would turn the build red. But if that happened, then the person landing the patch would see their new hazard mixed in with 2 others and wouldn't necessarily know which one needed to be fixed. It would be better to land this trivial patch to drop the hazards to zero, along with a patch to set the expected number of hazards to zero, so that any added hazards are immediately highlighted.

And yes, I should have done this in the first place, instead of waiting for the uplift. Sorry! I hadn't realized these were still there.
Attachment #8444508 - Flags: approval-mozilla-b2g32?
Attachment #8444508 - Flags: approval-mozilla-aurora?
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)

This landed on 33, which is what Aurora is now. Unless beta needs this for Android, you should be good with just b2g32.
Attachment #8444508 - Flags: approval-mozilla-aurora?
Comment on attachment 8444508 [details] [diff] [review]
Remove a function pointer (encodeVFP)

As the bug shows, this patch has been on m-c for 3 weeks and is quite small and looks to be pretty trivial. I'm going to take this small hygiene patch for 2.0.
Attachment #8444508 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
You need to log in before you can comment on or make changes to this bug.