Almost looking good.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc#newcode3240
src/hydrogen-instructions.cc:3240: HConstant* filler_map =
block->graph()->GetConstantFreeSpaceMap();
On 2013/07/08 13:49:38, titzer wrote:
On 2013/07/08 13:43:48, Hannes Payer wrote:
> On 2013/07/08 12:40:14, titzer wrote:
> > Don't make this a global constant in the graph. That increases its
live
range,
> > thus register pressure and spilling. It's fine to create this
constant at
each
> > allocation site, IMO. Maybe if you have multiple folded
allocations you can
> > insert this constant after the intial HAllocate and reuse the same
one, but
> > making it global is probably not worth it.
> OK.
>
> I am leaving this code since it will disappear anyway after fixing
the new
space
> iterator.
Why? If it is not so important, then please do not pollute HGraph with
another
global constant.
I kind of see Ben's point here. If it is possible to come up with a more
local temporary workaround (aka. hack) that would definitely be
preferable. Especially since one of Danno's upcoming changes will
actually track live-ranges for constants with more than one use (which
renders my original comment moot). So +1 on this comments as well.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h#newcode5811
src/hydrogen-instructions.h:5811: if (no_observable_side_effects) {
On 2013/07/08 13:49:38, titzer wrote:
On 2013/07/08 13:43:48, Hannes Payer wrote:
> On 2013/07/08 12:40:14, titzer wrote:
> > Please don't add more flags to this constructor; you should
probably set
these
> > flags in your special insertion routine. I'm not really sure why
you are
doing
> > this anyway...since I don't think the instructions are GVN'd after
that....
>
> This is needed otherwise the HGraph Verifier is not happy. But I
will add a
TODO
> as Michael suggested.
In either case, you should set the flag where you are creating the
instruction,
not in the constructor.
+1 on Ben's last comment.
https://codereview.chromium.org/18596005/diff/10001/src/hydrogen-gvn.cc
File src/hydrogen-gvn.cc (right):
https://codereview.chromium.org/18596005/diff/10001/src/hydrogen-gvn.cc#newcode800
src/hydrogen-gvn.cc:800: continue;
nit: The whole condition should fit into one line.
https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h#newcode237
src/hydrogen.h:237: : instr_(block->first()), next_(NULL) {
nit: Use a ternary operator, easier to read.
https://codereview.chromium.org/18596005/diff/10001/src/hydrogen.h#newcode247
src/hydrogen.h:247: if (!Done()) {
nit: Use a ternary operator, easier to read.
https://codereview.chromium.org/18596005/
--
--
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.
