Base iDef update code.

Base iDef update code.

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

[Massi, 2013-01-29 17:02:40]

Code to set up iDef def-use chains.
By itself it does nothing useful (and for now it is even disabled) but the new
check elimination code will be based on it.

[Jakob Kummerow, 2013-01-31 14:06:43]

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:379: while (next != NULL) {
I don't like endless loops very much.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:410: return
dominated->CheckFlag(kIDefsProcessingDone) || dominated->IsPhi();
I think this condition is the wrong way round: it's dominated if it has *not*
been visited yet and is *not* a Phi.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:873: static bool
CheckDominanceFollowingDefinitionList(
I'm not too happy with this method's name. "Check" sounds like some assertion.
Also, it would be nice to model this after HBasicBlock::Dominates(). Lastly, I'm
not sure what "FollowingDefinitionList" tries to tell me.
So how about "DominatesByBlockWalking"? And "DominatesByProcessedFlag" below? Or
just "DominatesSlow"/"DominatesFast"?

Ideally, I'd make it a non-static method too (so you can call it as
"dominator->DominatesSlow(other)", but that doesn't fit with passing method
pointers (see below). For templatized use, it would work if the template
parameter was a bool or enum used in an if/else (which the compiler would
optimize away).

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:874: HValue* dominator, HValue* dominated);
nit: for method/function declarations (contrary to calls), put each argument
onto its own line unless everything (including the method name) fits onto one
line. In this case:

  static bool CheckDominanceFollowingDefinitionList(HValue* dominator,
                                                    HValue* dominated);

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:876: // When we are setting up informaative
definitions we use a flag to mark
nit: s/informaative/informative/

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:880: HValue* dominator, HValue* dominated);
same here

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:884: template<bool (*CheckDominance)(HValue*,
HValue*)>
Please use a typedef for the function signature definition. (Put it right before
the first declaration of a function of this type; even though syntactically they
don't use it, they do semantically "implement the interface" defined by the
typedef.)

Is there any particular reason you templatized this method, as opposed to just
passing in a function pointer?

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:889: while (!uses.Done()) {
I'm used to seeing for-loops for iterating over uses. Let's be consistent with
that pattern:

for (HUseIterator uses = input->uses(); !uses.Done(); uses.Advance()) { ... }

https://codereview.chromium.org/12079042/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12079042/diff/1/src/hydrogen.cc#newcode3592
src/hydrogen.cc:3592: EliminateRedundantBoundsChecks();
nit: {} please

[Massi, 2013-01-31 17:16:19]

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:379: while (next != NULL) {
On 2013/01/31 14:06:43, Jakob wrote:
> I don't like endless loops very much.

Done (and it was actually an embarrassing bug...).

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:410: return
dominated->CheckFlag(kIDefsProcessingDone) || dominated->IsPhi();
On 2013/01/31 14:06:43, Jakob wrote:
> I think this condition is the wrong way round: it's dominated if it has *not*
> been visited yet and is *not* a Phi.

Done.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:873: static bool
CheckDominanceFollowingDefinitionList(
On 2013/01/31 14:06:43, Jakob wrote:
> I'm not too happy with this method's name. "Check" sounds like some assertion.
> Also, it would be nice to model this after HBasicBlock::Dominates(). Lastly,
I'm
> not sure what "FollowingDefinitionList" tries to tell me.
> So how about "DominatesByBlockWalking"? And "DominatesByProcessedFlag" below?
Or
> just "DominatesSlow"/"DominatesFast"?
> 
> Ideally, I'd make it a non-static method too (so you can call it as
> "dominator->DominatesSlow(other)", but that doesn't fit with passing method
> pointers (see below). For templatized use, it would work if the template
> parameter was a bool or enum used in an if/else (which the compiler would
> optimize away).

Renamed the methods but, after the offline chat, kept the template based
implementation.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:874: HValue* dominator, HValue* dominated);
On 2013/01/31 14:06:43, Jakob wrote:
> nit: for method/function declarations (contrary to calls), put each argument
> onto its own line unless everything (including the method name) fits onto one
> line. In this case:
> 
>   static bool CheckDominanceFollowingDefinitionList(HValue* dominator,
>                                                     HValue* dominated);

Done.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:876: // When we are setting up informaative
definitions we use a flag to mark
On 2013/01/31 14:06:43, Jakob wrote:
> nit: s/informaative/informative/

Done.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:880: HValue* dominator, HValue* dominated);
On 2013/01/31 14:06:43, Jakob wrote:
> same here

Done.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:884: template<bool (*CheckDominance)(HValue*,
HValue*)>
On 2013/01/31 14:06:43, Jakob wrote:
> Please use a typedef for the function signature definition. (Put it right
before
> the first declaration of a function of this type; even though syntactically
they
> don't use it, they do semantically "implement the interface" defined by the
> typedef.)
> 
> Is there any particular reason you templatized this method, as opposed to just
> passing in a function pointer?

Done.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:889: while (!uses.Done()) {
On 2013/01/31 14:06:43, Jakob wrote:
> I'm used to seeing for-loops for iterating over uses. Let's be consistent with
> that pattern:
> 
> for (HUseIterator uses = input->uses(); !uses.Done(); uses.Advance()) { ... }

Done.

https://codereview.chromium.org/12079042/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12079042/diff/1/src/hydrogen.cc#newcode3592
src/hydrogen.cc:3592: EliminateRedundantBoundsChecks();
On 2013/01/31 14:06:43, Jakob wrote:
> nit: {} please

Done.

[Jakob Kummerow, 2013-02-01 10:08:40]

LGTM

https://codereview.chromium.org/12079042/diff/2002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12079042/diff/2002/src/hydrogen-instructions....
src/hydrogen-instructions.h:888: template<DominanceTest CheckDominance>
s/Check/Test/ here too, please