Addressed feedback, PTAL again...
https://codereview.chromium.org/15769010/diff/11001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):
https://codereview.chromium.org/15769010/diff/11001/src/arm/lithium-arm.cc#newcode1502
src/arm/lithium-arm.cc:1502: UseFixedDouble(left, d1),
On 2013/06/07 19:55:03, Rodolph Perfetta wrote:
Technically not part of your patch but why are we using d1 and d2? The
hardfp
abi will use d0 and d1 so this will generate some extra moves.
The same happens on ia32 so I presume there is (was?) a reason. Note
that on
ia32 right is in d1 and left in d2 so only left will be moved before
the call.
We could do the same for ARM.
Good questions, but I don't have immediate answers to them. I'd like to
investigate this in a follow-up CL to get this CL into the tree as quick
as possible.
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#newcode1240
src/arm/lithium-codegen-arm.cc:1240: // Check for kMinInt % -1, sdiv
might signal an exception. We have to deopt
On 2013/06/07 19:55:03, Rodolph Perfetta wrote:
sdiv kMinInt, -1 will not trigger an exception but will return
kMinInt. (still
not what you want)
Adapted comment.
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.
On 2013/06/07 16:50:07, Jakob wrote:
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.
Removed the comment. "scary" = "it was a bug" ;-)
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.
On 2013/06/07 16:50:07, Jakob wrote:
IIUC, we don't need special handling for that case because we're being
clever
about sign handling. [...]
I added a comment about this.
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?
On 2013/06/07 19:55:03, Rodolph Perfetta wrote:
I don't think we do either.
in fact you should be able to always do the sub first and then if you
care about
-0 check if the result is 0 and left is negative (like it is done when
using
sdiv)
Good idea, I did that and simplified LModI accordingly.
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...
On 2013/06/07 16:50:07, Jakob wrote:
nit: I don't think this comment adds value.
I'll change this into a real TODO then, code like this is fundamentally
embarrassing and I really like to to this in cleaner way, but not in
this CL.
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
On 2013/06/07 16:50:07, Jakob wrote:
I'm not sure this comment adds value either.
Actually it does: This part of DoModI is basically a copy of the
has_fixed_right_arg() part, and this part will go away very soon, so I
wanted to make it clear that it doesn't make sense to unify things
somehow, this would even be counter-productive.
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());
On 2013/06/07 16:50:07, Jakob wrote:
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!
Yep, this clever handling has been there before, but most probably it
was pure luck. ;-) I'll add a comment as a reminder when we convert this
to Hydrogen-level strength reduction.
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.
