Turn array index dehoisting into a proper HPhase.

Turn array index dehoisting into a proper HPhase.

R=mvstanton@chromium.org

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

[mvstanton, 2013-07-11 11:58:21]

lgtm

[Benedikt Meurer, 2013-07-11 12:03:50]

Committed patchset #1 manually as r15627 (presubmit successful).

[titzer, 2013-07-11 12:23:23]

On 2013/07/11 12:03:50, Benedikt Meurer wrote:
> Committed patchset #1 manually as r15627 (presubmit successful).

Too late for this change, but could we pretty please have a solution that avoids
the proliferation of tiny hydrogen-*.h files that IMO serve no purpose.

I discussed this with Jakob and Sven a couple of weeks ago, where I suggested
just putting the HGraph::MyPhase methods in different files. We didn't agree on
a solution because there were too few datapoints at the time. Now we are getting
to having more than a dozen such headers. This is worrying.

I think we can get rid of these headers by just putting the HGraph::MyPhase
methods directly into the hydrogen-my-phase.cc files. Then any auxiliary classes
and datastructures that are necessary to that phase could be put directly in
there, without needing to have them in a header--any header. I know it is not
customary to put the implementation of a C++ class into multiple different
files, but I think in this case it would be manageable if we put a comment next
to the declaration of the HGraph::MyPhase() method indicating which file it is
in. This way we avoid a proliferation of tiny, and again, IMO, useless header
files. For the record, I do not consider "Because C++" a satisfactory response.

[Benedikt Meurer, 2013-07-11 12:27:17]

> Too late for this change, but could we pretty please have a solution that
avoids
> the proliferation of tiny hydrogen-*.h files that IMO serve no purpose.
> 
> I discussed this with Jakob and Sven a couple of weeks ago, where I suggested
> just putting the HGraph::MyPhase methods in different files. We didn't agree
on
> a solution because there were too few datapoints at the time. Now we are
getting
> to having more than a dozen such headers. This is worrying.
> 
> I think we can get rid of these headers by just putting the HGraph::MyPhase
> methods directly into the hydrogen-my-phase.cc files. Then any auxiliary
classes
> and datastructures that are necessary to that phase could be put directly in
> there, without needing to have them in a header--any header. I know it is not
> customary to put the implementation of a C++ class into multiple different
> files, but I think in this case it would be manageable if we put a comment
next
> to the declaration of the HGraph::MyPhase() method indicating which file it is
> in. This way we avoid a proliferation of tiny, and again, IMO, useless header
> files. For the record, I do not consider "Because C++" a satisfactory
response.

I asked Danno about the two files per phase issue before; he said I should just
do it, as we're going to put all the compiler related files into a separate
subdirectory. IMHO, we should put all the Hydrogen phases into
hydrogen-phases.{cc,h}. That'd be consistent with hydrogen-instructions.{cc,h}.

[Jakob Kummerow, 2013-07-11 12:43:31]

On 2013/07/11 12:27:17, Benedikt Meurer wrote:
> I asked Danno about the two files per phase issue before; he said I should
just
> do it, as we're going to put all the compiler related files into a separate
> subdirectory. IMHO, we should put all the Hydrogen phases into
> hydrogen-phases.{cc,h}. That'd be consistent with
hydrogen-instructions.{cc,h}.

I don't mind those tiny headers. Hopefully we'll rarely have to open them in an
editor, and if we hide them in src/hydrogen/ their existence wouldn't bother me.

Yet another suggestion:
src/hydrogen/phases.h  <-- #included by hydrogen.cc, defines the public
interface of all phases
src/hydrogen/phase-gvn.cc  <-- implementation of just one phase; file names make
the 1:n coupling of .h/.cc obvious
src/hydrogen/phase-dehoist.cc
etc.
Bonus points if someone comes up with a sensible renaming of the phases so that
they're executed in alphabetical order :-P

But I'm fine with Ben's suggestion too.

I'd go as far as suggesting that we should split HInstructions' implementations
across .cc files too: 
all the HFoo::RepresentationFromInputs() implementations should be in
hydrogen/infer-representation.cc,
HFoo::Canonicalize() should be in hydrogen/canonicalize.cc, and so on.

[Benedikt Meurer, 2013-07-11 13:13:24]

On 2013/07/11 12:43:31, Jakob wrote:
> On 2013/07/11 12:27:17, Benedikt Meurer wrote:
> > I asked Danno about the two files per phase issue before; he said I should
> just
> > do it, as we're going to put all the compiler related files into a separate
> > subdirectory. IMHO, we should put all the Hydrogen phases into
> > hydrogen-phases.{cc,h}. That'd be consistent with
> hydrogen-instructions.{cc,h}.
> 
> I don't mind those tiny headers. Hopefully we'll rarely have to open them in
an
> editor, and if we hide them in src/hydrogen/ their existence wouldn't bother
me.
> 
> Yet another suggestion:
> src/hydrogen/phases.h  <-- #included by hydrogen.cc, defines the public
> interface of all phases
> src/hydrogen/phase-gvn.cc  <-- implementation of just one phase; file names
make
> the 1:n coupling of .h/.cc obvious
> src/hydrogen/phase-dehoist.cc
> etc.
> Bonus points if someone comes up with a sensible renaming of the phases so
that
> they're executed in alphabetical order :-P

1-environment-liveness.cc
2-redundant-phis.cc
3-dce.cc
...

What can I do with my bonus points? :-)

[danno, 2013-07-11 14:08:34]

Ben's observation seems reasonable, the .cc split is fine but the tiny headers
are annoying. I like Jakob's solution, where all of the phases are declared
together but the implementations are split out.

[Dmitry Lomov (no reviews), 2013-07-12 07:56:55]

On 2013/07/11 14:08:34, danno wrote:
> Ben's observation seems reasonable, the .cc split is fine but the tiny headers
> are annoying. I like Jakob's solution, where all of the phases are declared
> together but the implementations are split out.

DBC: I do not feel this strongly, but I actually like small headers - it is
really easy to switch between declaration and definition, and having small
headers limits the required recompilation when something changes. My vote would
be to keep small headers.