Turn range analysis into proper HPhase.

Turn range analysis into proper HPhase.

R=dslomov@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15496

[Dmitry Lomov (no reviews), 2013-07-03 19:37:59]

https://codereview.chromium.org/18491002/diff/1/src/hydrogen-range-analysis.cc
File src/hydrogen-range-analysis.cc (right):

https://codereview.chromium.org/18491002/diff/1/src/hydrogen-range-analysis.c...
src/hydrogen-range-analysis.cc:35: if (FLAG_trace_range) {
Since HRangeAnalysisPhase is a proper CompilationPhase now, it should use
CompilationPhase::ShouldProduceTraceOutput() to determine whether to produce
trace output, and FLAG_trace_range should go away.

[Benedikt Meurer, 2013-07-04 06:42:04]

https://codereview.chromium.org/18491002/diff/1/src/hydrogen-range-analysis.cc
File src/hydrogen-range-analysis.cc (right):

https://codereview.chromium.org/18491002/diff/1/src/hydrogen-range-analysis.c...
src/hydrogen-range-analysis.cc:35: if (FLAG_trace_range) {
On 2013/07/03 19:37:59, Dmitry Lomov (chromium) wrote:
> Since HRangeAnalysisPhase is a proper CompilationPhase now, it should use
> CompilationPhase::ShouldProduceTraceOutput() to determine whether to produce
> trace output, and FLAG_trace_range should go away. 

Of course, but this way we would be unable to trace only the range analysis
however. Is that what we want?

[Dmitry Lomov (no reviews), 2013-07-04 15:10:34]

On 2013/07/04 06:42:04, Benedikt Meurer wrote:
> https://codereview.chromium.org/18491002/diff/1/src/hydrogen-range-analysis.cc
> File src/hydrogen-range-analysis.cc (right):
> 
>
https://codereview.chromium.org/18491002/diff/1/src/hydrogen-range-analysis.c...
> src/hydrogen-range-analysis.cc:35: if (FLAG_trace_range) {
> On 2013/07/03 19:37:59, Dmitry Lomov (chromium) wrote:
> > Since HRangeAnalysisPhase is a proper CompilationPhase now, it should use
> > CompilationPhase::ShouldProduceTraceOutput() to determine whether to produce
> > trace output, and FLAG_trace_range should go away. 
> 
> Of course, but this way we would be unable to trace only the range analysis
> however. 

No I don't think so - see the source for ShouldProduceTraceOutput:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/compiler.cc...

The combination '--trace-hydrogen --trace-phase="H_Range analysis"' will trace
only range analysis

[Benedikt Meurer, 2013-07-04 15:13:43]

> > > Since HRangeAnalysisPhase is a proper CompilationPhase now, it should use
> > > CompilationPhase::ShouldProduceTraceOutput() to determine whether to
produce
> > > trace output, and FLAG_trace_range should go away. 
> > 
> > Of course, but this way we would be unable to trace only the range analysis
> > however. 
> 
> No I don't think so - see the source for ShouldProduceTraceOutput:
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/compiler.cc...
> 
> The combination '--trace-hydrogen --trace-phase="H_Range analysis"' will trace
> only range analysis

It uses StrChr to see if the first character of the phase name (H in case of a
hydrogen phase) appears in FLAG_trace_phase.

[Dmitry Lomov (no reviews), 2013-07-04 15:26:32]

On 2013/07/04 15:13:43, Benedikt Meurer wrote:
> > > > Since HRangeAnalysisPhase is a proper CompilationPhase now, it should
use
> > > > CompilationPhase::ShouldProduceTraceOutput() to determine whether to
> produce
> > > > trace output, and FLAG_trace_range should go away. 
> > > 
> > > Of course, but this way we would be unable to trace only the range
analysis
> > > however. 
> > 
> > No I don't think so - see the source for ShouldProduceTraceOutput:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/compiler.cc...
> > 
> > The combination '--trace-hydrogen --trace-phase="H_Range analysis"' will
trace
> > only range analysis
> 
> It uses StrChr to see if the first character of the phase name (H in case of a
> hydrogen phase) appears in FLAG_trace_phase.

Oh I am sorry - you are right (I guess there was some sand in my eyes :))
Let's have a (FLAG_trace_range || ShouldProduceTraceOutput()) here.

LGTM with that change.

[Benedikt Meurer, 2013-07-04 15:31:00]

> Let's have a (FLAG_trace_range || ShouldProduceTraceOutput()) here.

IMHO we should be consistent in wrt the phase specific flags. Your proposed
change should be applied to all hydrogen phases w/ special trace flags.
Opinions?

[Dmitry Lomov (no reviews), 2013-07-04 15:40:38]

On 2013/07/04 15:31:00, Benedikt Meurer wrote:
> > Let's have a (FLAG_trace_range || ShouldProduceTraceOutput()) here.
> 
> IMHO we should be consistent in wrt the phase specific flags. Your proposed
> change should be applied to all hydrogen phases w/ special trace flags.
> Opinions?

Ok I took a closer look, you are quite right - we do not consistently use
ShouldProduceTraceOutput().
So this shouldn't block you from landing this CL, so lgtm.

[Benedikt Meurer, 2013-07-04 19:25:23]

Committed patchset #2 manually as r15496 (presubmit successful).