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#newcode379
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#newcode410
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#newcode873
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#newcode874
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#newcode876
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#newcode880
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#newcode884
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#newcode889
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.
https://codereview.chromium.org/12079042/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
