Haven't looked at MIPS.
https://codereview.chromium.org/15769010/diff/11001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/15769010/diff/11001/src/arm/lithium-codegen-arm.cc#newcode1303
src/arm/lithium-codegen-arm.cc:1303: // TODO(svenpanne) The last
comments seems to be wrong nowadays.
Delete them, then!
"The divisor value is preloaded before" was true before you changed the
code (divisor was loaded in line 1252, comment was in line 1273).
"Be careful that 'right_reg' is only live on entry." clearly does not
match "UseRegister(right)" in lithium-arm.cc; probably it was meant to
indicate that |right| got clobbered in line 1269. Which is scary because
|right| wasn't allocated as UseTempRegister.
https://codereview.chromium.org/15769010/diff/11001/src/arm/lithium-codegen-arm.cc#newcode1311
src/arm/lithium-codegen-arm.cc:1311: // this correct? If yes, a comment
about this might be appropriate.
IIUC, we don't need special handling for that case because we're being
clever about sign handling. It seems what happens is:
vabs -> divisor = 1
vdiv -> quotient = kMinInt / 1 = kMinInt
vcvt, vcvt -> quotient == kMinInt still
vmul -> double_scratch = 1 * kMinInt = kMinInt
vcvt, vmov -> scratch = kMinInt
sub -> result_reg = kMinInt - kMinInt = 0
So no overflow anywhere along the chain.
https://codereview.chromium.org/15769010/diff/11001/src/arm/lithium-codegen-arm.cc#newcode1330
src/arm/lithium-codegen-arm.cc:1330: // TODO(svenpanne) Why do we need
scratch2 in addition to scratch?
I don't think we do.
https://codereview.chromium.org/15769010/diff/11001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/15769010/diff/11001/src/hydrogen-instructions.h#newcode1042
src/hydrogen-instructions.h:1042: // We should really use the null
object pattern here...
nit: I don't think this comment adds value.
https://codereview.chromium.org/15769010/diff/11001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/15769010/diff/11001/src/ia32/lithium-codegen-ia32.cc#newcode1241
src/ia32/lithium-codegen-ia32.cc:1241: // TODO(svenpanne) We should
really do the strength reduction on the
I'm not sure this comment adds value either.
https://codereview.chromium.org/15769010/diff/11001/src/ia32/lithium-codegen-ia32.cc#newcode1246
src/ia32/lithium-codegen-ia32.cc:1246: int32_t divisor =
Abs(right->GetInteger32Constant());
This just works by accident for right->GetInteger32Constant() ==
kMinInt, eh? |divisor| will be 0 due to overflow; if left_reg contains
anything other than kMinInt the operation should be a no-op and the
sequence "neg, and_(..., -1), neg" thankfully is, and if left_reg does
contain kMinInt, then the first "neg" turns it to 0 and the other
instructions don't modify it further. Phew!
https://codereview.chromium.org/15769010/
--
--
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.
