Last Comment Bug 668660 - IM: LICM - hoist loop invariant instructions
: IM: LICM - hoist loop invariant instructions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on: 669517
Blocks: 664547
  Show dependency treegraph
 
Reported: 2011-06-30 15:48 PDT by Andrew Scheff
Modified: 2012-01-30 13:21 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This patch adds code hoisting functionality (3.09 KB, patch)
2011-06-30 15:48 PDT, Andrew Scheff
dvander: review+
Details | Diff | Splinter Review
Improved code hoisting (4.62 KB, patch)
2011-07-05 16:01 PDT, Andrew Scheff
dvander: review+
Details | Diff | Splinter Review

Description Andrew Scheff 2011-06-30 15:48:02 PDT
Created attachment 543284 [details] [diff] [review]
This patch adds code hoisting functionality
Comment 1 David Anderson [:dvander] 2011-07-01 13:41:26 PDT
Comment on attachment 543284 [details] [diff] [review]
This patch adds code hoisting functionality

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

Looks good, r=me with changes

::: js/src/ion/LICM.h
@@ +102,5 @@
> +    // only if they allow another instruction that is a BIG_WIN to be hoisted as well
> +    static const int NO_WIN = 0;
> +    static const int POTENTIAL_WIN = 1;
> +    static const int BIG_WIN = 2;
> +    int getHoistWin(MInstruction *ins);

Make this an enum instead. Function name is a little confusing - how about "estimateHoistingWin"?

@@ +103,5 @@
> +    static const int NO_WIN = 0;
> +    static const int POTENTIAL_WIN = 1;
> +    static const int BIG_WIN = 2;
> +    int getHoistWin(MInstruction *ins);
> +    bool checkPotentialWin(MInstruction *ins);

How about "shouldHoist"?
Comment 2 Andrew Scheff 2011-07-05 16:01:49 PDT
Created attachment 544090 [details] [diff] [review]
Improved code hoisting

Addresses comments, as well as:

Added enum to MInstruction, instructions are now classified by how they should be considered for hoisting

Added method that checks for block hotness, with the eventual goal of not hoisting instructions from cold blocks inside of loops.  How this will be done is TBD.
Comment 3 David Anderson [:dvander] 2011-07-05 16:27:40 PDT
Comment on attachment 544090 [details] [diff] [review]
Improved code hoisting

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

::: js/src/ion/LICM.cpp
@@ +248,5 @@
> +
> +bool
> +Loop::checkHotness(MBasicBlock *block)
> +{
> +    // TODO

TODO should be a filed a bug w/ explanation, otherwise it will get forgotten :)

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