Move BailoutReason and flags computation to post-pass

Move BailoutReason and flags computation to post-pass

R=svenpanne@chromium.org

Committed: https://chromium.googlesource.com/v8/v8/+/910711a16963aebccef59a2f6bf8c7371d985596

[wingo, 2014-10-22 16:22:57]

this one builds on https://codereview.chromium.org/675493002/, so please look at
that one first

[wingo, 2014-10-22 16:52:01]

On 2014/10/22 16:22:57, wingo wrote:
> this one builds on https://codereview.chromium.org/675493002/, so please look
at
> that one first

Hum, I seem to have uploaded a cumulative CL.  Please don't review until I fix,
thanks.

[wingo, 2014-10-22 16:52:10]

wingo@igalia.com changed reviewers:
- svenpanne@chromium.org

[wingo, 2014-10-23 07:08:23]

Patchset #1 (id:1) has been deleted

[wingo, 2014-10-23 07:08:56]

wingo@igalia.com changed reviewers:
+ svenpanne@chromium.org

[wingo, 2014-10-23 07:08:56]

Fixed, PTAL after the node counting CL mentioned above is done.

[wingo, 2014-10-24 09:50:23]

On 2014/10/23 07:08:56, wingo wrote:
> Fixed, PTAL after the node counting CL mentioned above is done.

I've rebased the patch on top of the rebased node-counting CL, so probably you
want to look at the overall diffs.

[wingo, 2014-10-24 09:51:50]

https://codereview.chromium.org/668143003/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/668143003/diff/40001/src/compiler.cc#newcode934
src/compiler.cc:934: if (!Renumber(&unoptimized)) return false;
whoops, this crept back in

[wingo, 2014-10-24 10:36:42]

https://codereview.chromium.org/668143003/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/668143003/diff/40001/src/compiler.cc#newcode934
src/compiler.cc:934: if (!Renumber(&unoptimized)) return false;
On 2014/10/24 09:51:50, wingo wrote:
> whoops, this crept back in

Done.

[wingo, 2014-10-27 15:08:20]

https://codereview.chromium.org/668143003/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/668143003/diff/60001/src/compiler.cc#newcode746
src/compiler.cc:746:
info->shared_info()->set_bailout_reason(lit->dont_optimize_reason());
As mstarzinger points out, this should probably be
SharedFunctionInfo::DisableOptimization().

[wingo, 2014-10-27 15:55:38]

New patch uses DisableOptimization().  PTAL, this one is next in line :)

[Sven Panne, 2014-10-29 09:12:55]

https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc#new...
src/ast-numbering.cc:48: properties_.flags()->Add(kDontSelfOptimize);
Can we delegate between all these helpers and rely on inlining for performance?
This would make the implications much clearer. In this case:

   dont_crankshaft_reason_ = reason;
   DisableSelfOptimization();

https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc#new...
src/ast-numbering.cc:59: properties_.add_node_count(1);
Huh? Why do we increment again here (and in DisableCaching)? This looks wrong...

https://codereview.chromium.org/668143003/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/668143003/diff/80001/src/compiler.cc#newcode581
src/compiler.cc:581: static void
MaybeDisableOptimization(Handle<SharedFunctionInfo> shared_info,
I think this should be folded into DisableOptimization, which should probably be
renamed then. Hmmm, SetBailoutReason makes sense, but we already have
set_bailout_reason in that class (with a getter DisableOptimizationReason,
*very* consistent naming :-P). OK, so my proposal would be:

   * Rename DisableOptimization to SetBailoutReason and include the kNoReason
handling there.

   * Rename set_bailout_reason to set_disable_optimization_reason and
DisableOptimizationReason to disable_optimization_reason.

[wingo, 2014-11-12 08:50:51]

Thanks for the review and sorry for the delay.  The updated patch addresses
nits.

https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc#new...
src/ast-numbering.cc:48: properties_.flags()->Add(kDontSelfOptimize);
On 2014/10/29 09:12:55, Sven Panne wrote:
> Can we delegate between all these helpers and rely on inlining for
performance?
> This would make the implications much clearer. In this case:
> 
>    dont_crankshaft_reason_ = reason;
>    DisableSelfOptimization();

Done.

https://codereview.chromium.org/668143003/diff/80001/src/ast-numbering.cc#new...
src/ast-numbering.cc:59: properties_.add_node_count(1);
On 2014/10/29 09:12:55, Sven Panne wrote:
> Huh? Why do we increment again here (and in DisableCaching)? This looks
wrong...

Yes, a bug from a previous version of the patch.  Good catch!  I re-ran
MandreelLatency with this patch applied on master and fixing this problem didn't
have any perf impact, which I guess makes sense as the node count is used for
inlining and these cases wouldn't be optimized anyway.

https://codereview.chromium.org/668143003/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/668143003/diff/80001/src/compiler.cc#newcode581
src/compiler.cc:581: static void
MaybeDisableOptimization(Handle<SharedFunctionInfo> shared_info,
On 2014/10/29 09:12:55, Sven Panne wrote:
> I think this should be folded into DisableOptimization, which should probably
be
> renamed then. Hmmm, SetBailoutReason makes sense, but we already have
> set_bailout_reason in that class (with a getter DisableOptimizationReason,
> *very* consistent naming :-P). OK, so my proposal would be:
> 
>    * Rename DisableOptimization to SetBailoutReason and include the kNoReason
> handling there.
> 
>    * Rename set_bailout_reason to set_disable_optimization_reason and
> DisableOptimizationReason to disable_optimization_reason.

So, I did the renames to {set_,}disable_optimization_reason then looked at
DisableOptimization.  Thing is, DisableOptimization does not just update flags
in the shared function info -- it also updates flags on the code object, and the
code object can change.  I think MaybeDisableOptimization is the probably the
right solution here.  For sanity I added an assert to DisableOptimization that
the reason was not kNoReason.  WDYT?

[wingo, 2014-11-12 13:59:29]

FWIW, I ran the MandreelLatency test on this patch as applied to master and
results were better than they were before the start of this patch series.  Ready
to go in whenever you like :)

[Sven Panne, 2014-11-12 15:44:37]

LGTM, let's see what breaks... :-D

[wingo, 2014-11-13 09:34:46]

Committed patchset #6 (id:120001) manually as
6d99e6701aa261c843391bc75654d3cda76ed499 (presubmit successful).

[wingo (chromium), 2014-11-13 09:41:48]

On 2014/11/13 09:34:46, wingo wrote:
> Committed patchset #6 (id:120001) manually as
> 6d99e6701aa261c843391bc75654d3cda76ed499 (presubmit successful).

Er, I thought we switched to git already as my "git cl dcommit" wasn't working,
so i did a "git cl land" and it claimed to work, but it hasn't hit the remote
repo.  Will poke to see what went wrong...

[wingo (chromium), 2014-11-13 09:45:58]

On 2014/11/13 09:41:48, wingo (chromium) wrote:
> On 2014/11/13 09:34:46, wingo wrote:
> > Committed patchset #6 (id:120001) manually as
> > 6d99e6701aa261c843391bc75654d3cda76ed499 (presubmit successful).
> 
> Er, I thought we switched to git already as my "git cl dcommit" wasn't
working,
> so i did a "git cl land" and it claimed to work, but it hasn't hit the remote
> repo.  Will poke to see what went wrong...

Seems I was using the github mirror like a fool!  Reopening and will land after
I've fixed my branches :P

[wingo, 2014-11-13 09:58:03]

Committed patchset #6 (id:120001) manually as
910711a16963aebccef59a2f6bf8c7371d985596 (presubmit successful).