This is generally the right direction, but I have some suggestions for
simplification.
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#newcode818
src/hydrogen-instructions.cc:818: HBoundsCheck* HBoundsCheck::AddAfter(
I don't see this used anywhere.
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#newcode842
src/hydrogen-instructions.cc:842: HDeoptimizeIfTaggedIsNotSmi*
checked_length =
Didn't we agree that HBoundsCheck is only ever used for arrays with FAST
elements, and those always have a Smi as their length? So I don't think
we need this check.
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.cc#newcode2973
src/hydrogen-instructions.cc:2973: HInstruction::Verify();
This is the default implementation, no need to override it.
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#newcode2616
src/hydrogen-instructions.h:2616: class HDeoptimizeIfTaggedIsNotSmi:
public HUnaryOperation {
For consistency with HCheckSmi, I'd call this HCheckSmiOrInt32.
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#newcode2618
src/hydrogen-instructions.h:2618: explicit HDeoptimizeIfTaggedIsNotSmi(
no "explicit" necessary (unless you remove the Representation argument,
see below).
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#newcode2626
src/hydrogen-instructions.h:2626: } else {
I don't think we need this case. This instruction doesn't do anything
when the representation is Integer32, and in the code stubs case it is
always Integer32. So we can simply not insert it at all in that case.
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#newcode3128
src/hydrogen-instructions.h:3128: class HGraphBuilder;
Please don't do this. Instead, move the insertion helper function to the
graph, similar to HGraphBuilder::AddSimulate or
HOptimizedGraphBuilder::AddSoftDeoptimize(). (If we need the helper at
all -- see my other comments.)
https://codereview.chromium.org/12208013/diff/1/src/hydrogen-instructions.h#newcode3165
src/hydrogen-instructions.h:3165: : key_mode_(key_mode),
skip_check_(false) {
nit: more indentation
https://codereview.chromium.org/12208013/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):
https://codereview.chromium.org/12208013/diff/1/src/ia32/lithium-ia32.h#newcode2279
src/ia32/lithium-ia32.h:2279: class LDeoptimizeIfTaggedIsNotSmi: public
LTemplateInstruction<0, 1, 0> {
I think we don't need a new LInstruction.
Instead, I think the "representation.IsTagged() && !type.IsSmi()" logic
should go into the Canonicalization phase, i.e. the
HDeoptimizeIfTaggedIsNotSmi should just remove itself when it is not
needed. When it is needed, we can simply translate it to an LCheckSmi.
Or am I missing something?
https://codereview.chromium.org/12208013/
--
--
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.
