As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 710447 - IonMonkey: Invalid hoisting in basic/testBug529147.js
: IonMonkey: Invalid hoisting in basic/testBug529147.js
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 703376
  Show dependency treegraph
 
Reported: 2011-12-13 15:55 PST by Sean Stangl [:sstangl]
Modified: 2011-12-13 16:16 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Set NeverHoisted on MLoadSlots. (655 bytes, patch)
2011-12-13 16:00 PST, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2011-12-13 15:55:42 PST
basic/testBug529147.js returns an incorrect value due to invalid hoisting. The code looks something like this:

> var sum = 0; // global
> function foo(n) {
>   for (var i = 0; i < n; i++)
>     sum += 10;
> }

Around the global addition, MLoadSlot is marked as idempotent and movable. We hoist the global load and add out of the loop, then repeatedly store the same old value.

MLoadSlot should not be movable. (We can hoist it in some conditions -- but the analysis would be difficult or impossible if there is a storeslot or function call within the loop.)
Comment 1 User image Sean Stangl [:sstangl] 2011-12-13 16:00:28 PST
Created attachment 581466 [details] [diff] [review]
Set NeverHoisted on MLoadSlots.

We can't hoist unless it's provable that the loop cannot change the slot's value. Fixes test failure.
Comment 2 User image Brian Hackett (:bhackett) 2011-12-13 16:01:57 PST
This depends on having alias sets (bug 703376), right?  Associate the MLoadSlot with a TypeObject/property if possible, and then it is movable/hoistable just so long as it is not moved across an operation which could overwrite the type/property.
Comment 3 User image David Anderson [:dvander] 2011-12-13 16:10:13 PST
Comment on attachment 581466 [details] [diff] [review]
Set NeverHoisted on MLoadSlots.

bug 703376 should fix this and let us hoist MLoadSlot when possible - it's okay to land this to clear up orange if we take out the NeverHoisted bit as soon as bug 703376 lands.
Comment 4 User image Sean Stangl [:sstangl] 2011-12-13 16:11:25 PST
(In reply to Brian Hackett (:bhackett) from comment #2)
> This depends on having alias sets (bug 703376), right?  Associate the
> MLoadSlot with a TypeObject/property if possible, and then it is
> movable/hoistable just so long as it is not moved across an operation which
> could overwrite the type/property.

Yes. We should keep the default state unhoistable, which alias sets can disprove. In the current state, we just hoist optimistically without reason, which is causing a number of test failures.

The test failures showed up only now due to greater test coverage from Bug 708441.
Comment 5 User image Sean Stangl [:sstangl] 2011-12-13 16:16:14 PST
http://hg.mozilla.org/projects/ionmonkey/rev/1c2192196b11

Note You need to log in before you can comment on or make changes to this bug.