Thank you for the next patch, sorry it took so long to respond to it, but
it's
quite a tricky problem how to design a clear API for supporting both 31-
and 32-
bit smis and still keeping a clear, simple division of responsibilities.
In my review, I focused on your changes to the macro assembler and the
wrapper
classes (which I think should be renamed to have a super class called
SmiOperationOverflowHandler). The changes I reviewed are going in the right
direction but I suspect will still need several iterations.
I encourage you to break out the unrelated changed, as you already have for
some, to make landing those easier.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/full-codegen-x64.cc#newcode4454
src/x64/full-codegen-x64.cc:4454: __ SmiAddConstant(rax, rax,
Smi::FromInt(1));
Shouldn't overflow also be handled in SmiAddConstant by passing in a
wrapper?
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc
File src/x64/macro-assembler-x64.cc (right):
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc#newcode990
src/x64/macro-assembler-x64.cc:990: static inline Immediate
SmiToImmediate(Smi* src) {
Why not add a constructor to the Immeidate constructor in the x64
assembler that takes a Smi as a parameter? It can ASSERT that it only
called if SmiValuesAre31Bits() is true (no if) and then return the
properly initialized Immediate value directly. That approach makes this
helper function unnecessary.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc#newcode1705
src/x64/macro-assembler-x64.cc:1705: if
(wrapper.NeedsKeepSourceOperandsIntact()) {
Here and everywhere else for the Add/Sub operations. Why not define the
interface as taking a separate parameter (not the wrapper) that
specifies if the call site cares about clobbering src1 if it's dst on
the overflow case. The code can then use the trick that you've seen in
the stub in your other CL:
addq(src1, src2);
j(no_overflow, local_done);
subq(src1, src2);
wrapper.BailoutIf(always);
bind(local_done);
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc#newcode1955
src/x64/macro-assembler-x64.cc:1955: const SmiInstructionWrapper&
wrapper) {
This code seems identical to the version directly above. Can you
templatize the core and just have two wrappers that call either with a
Register or Operand?
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc#newcode1966
src/x64/macro-assembler-x64.cc:1966: if (wrapper.NeedsCheckOverflow())
wrapper.BailoutIf(overflow);
Always call BaikoutIf, here and elsewhere.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc#newcode2067
src/x64/macro-assembler-x64.cc:2067: const SmiInstructionWrapper&
wrapper) {
This code seems identical to the version directly above. Can you
templatize the core and just have two wrappers that call either with a
Register or Operand?
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.cc#newcode2497
src/x64/macro-assembler-x64.cc:2497: ASSERT(!dst.is(rcx));
Here and elsewhere, I think it really makes testability more difficult
if the register constraints are different between 32- and 31-bit smis.
For now, you can ASSERT(!dst.is(src1)) in all cases, that should work
for full-code-gen and stub usages.
Alternatively (maybe better from an abstraction point of view, but
probably outside the scope of this CL), you can always pass an extra
scratch register (like r9) to SmiShiftLogicalRight and the other
routines so that dst can actually be src1 and things still work (that
removes the need for the special handling of the dst register at the
caller site that you added).
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.h#newcode378
src/x64/macro-assembler-x64.h:378: class SmiInstructionWrapper {
My general feedback to this class is that it's too complex. You seem to
be mixing responsibilities. e.g. whether a particular SMI operation
needs to check for overflow is a decision of the calling site of the
macro instruction, less so a policy of whether you're generating full
code gen code or not. Same for NeedsCheckMinusZero, it seems like
something that is passed in separately.
How about this instead (including a rename thatI think is important):
class SmiOperationOverflowHandler {
public:
virtual void BailoutIf(Condition cc) const = 0;
}
// Use this for callers that don't care about overflow at all.
class IgnoreSmiOverflowHandler : public SmiOperationOverflowHandler{
virtual void BailoutIf(Condition cc) const {}
}
class JumpOnSmiOverflowHandler : public SmiOperationOverflowHandler {
virtual void BailoutIf(Condition cc) const { DeoptimizeIf(cc, env) }
}
class DeoptOnSmiOverflowHandler : public SmiOperationOverflowHandler {
SmiOperationOverflowHandler(bool can_overflow) {...}
virtual void BailoutIf(Condition cc) const { if (can_overflow_) {
DeoptimizeIf(cc, env); } }
}
NeedsKeepSourceOperandsIntact also doesn't seem to belong here. It seems
that there are two different types of Smi operations. Some, like Add and
Sub, have some callers that want to make sure that their input register
are preserved in certain situations... this is a per-call-site decision.
Others, like SmiShiftLeft, didn't care if dst is the same as src1/src2,
clobbering was OK. That should still remain the case when you add 31-bit
smi support, which might mean that you need to pass in an addition temp
register so that you don't have to write different or convoluted code at
the caller site in the 31/32 bit smi cases.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.h#newcode383
src/x64/macro-assembler-x64.h:383: virtual bool NeedsCheckMinusZero()
const = 0;
You never call NeedsCheckMinusZero anywhere. Can you remove it.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.h#newcode384
src/x64/macro-assembler-x64.h:384: virtual bool
NeedsKeepSourceOperandsIntact() const = 0;
I think you can avoid this one, too. See my comments in the .cc file.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.h#newcode388
src/x64/macro-assembler-x64.h:388: class SafeSmiInstructionWrapper :
public SmiInstructionWrapper {
This class is never used, please remove it.
https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-assembler-x64.h#newcode428
src/x64/macro-assembler-x64.h:428: bool IsUnsafeInt(const int x);
shouldn't this be a int32_t just to be clear?
https://chromiumcodereview.appspot.com/21014003/
--
--
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.
