General comments:
1. The .gyp project file needs to be updated for the added files.
2. We have to figure out some way that an adversary (err, benchmark writer)
cannot force us to do arbitrary amounts of useless work only to bail out on
a
non-number input.
Why not at least have the syntax checker compute a set of vars that need to
be
numbers in order for the whole expression to succeed, and then hoist the
number
checks to the top? That seems like it would also avoid redundantly
checking the
same inputs.
http://codereview.chromium.org/660077/diff/1038/52
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/660077/diff/1038/52#newcode4749
src/ia32/codegen-ia32.cc:4749: SafeSyntaxChecker checker;
As far as I can see, the following 20+ lines are repeated six times in
this file. Please abstract it into a function.
http://codereview.chromium.org/660077/diff/1038/50
File src/ia32/safe-codegen-ia32.cc (right):
http://codereview.chromium.org/660077/diff/1038/50#newcode69
src/ia32/safe-codegen-ia32.cc:69: void
SafeSyntaxChecker::VisitBlock(Block* stmt) {
Don't bother writing all these out unless you will someday implement
them.
#define DEFINE_STMT_VISIT(type) \
void SafeSyntaxChecker::Visit##type(type* stmt) { \
UNREACHABLE(); \
}
STATEMENT_NODE_LIST(DEFINE_STMT_VISIT)
#undef DEFINE_STMT_VISIT
http://codereview.chromium.org/660077/diff/1038/50#newcode175
src/ia32/safe-codegen-ia32.cc:175: // Check that slot is local or
context.
It's confusing when comments and code don't agree. Here the code is
checking that the slot has type LOCAL or PARAMETER, not CONTEXT.
http://codereview.chromium.org/660077/diff/1038/50#newcode176
src/ia32/safe-codegen-ia32.cc:176: if ((expr->type() == Slot::LOCAL &&
!expr->is_arguments()) ||
Why not arguments? It's not out of the question that a programmer could
shadow it or assign to it, and I can't see here why this optimization
should be disabled for those cases.
Is it because we can't cope with the frame effect of lazy arguments
initialization?
In any case, it needs a simple comment.
http://codereview.chromium.org/660077/diff/1038/50#newcode178
src/ia32/safe-codegen-ia32.cc:178: if (++xmm_stack_height_ >
MAX_XMM_STACK_HEIGHT) {
I don't like side effects embedded in expressions unless there's no
simple way around it.
It's too easy to read the expression as merely checking the bailout
condition and miss that it has a side effects executed on all paths.
I'm sure the compiler will generate good code for:
xmm_stack_height_++;
if (xmm_stack_height_ > MAX_XMM_STACK_HEIGHT) { ...
http://codereview.chromium.org/660077/diff/1038/50#newcode179
src/ia32/safe-codegen-ia32.cc:179: BAILOUT("XMM stack height too
great");
"too great"? How about "overflow" so it doesn't sound like a value
judgement.
http://codereview.chromium.org/660077/diff/1038/50#newcode182
src/ia32/safe-codegen-ia32.cc:182: BAILOUT("Slot has type CONTEXT or
LOOKUP");
I don't understand why this optimization doesn't apply to context slots.
It might seem picky, but since there's always a chance that we'll add
other slot types (eg, GLOBAL), you should say something like
"Non-local/non-parameter slot".
http://codereview.chromium.org/660077/diff/1038/50#newcode200
src/ia32/safe-codegen-ia32.cc:200: if (++xmm_stack_height_ >
MAX_XMM_STACK_HEIGHT) {
Same comment about side effects embedded in expressions.
http://codereview.chromium.org/660077/diff/1038/50#newcode327
src/ia32/safe-codegen-ia32.cc:327: // Supported.Visit(expr->left());
Huh?
http://codereview.chromium.org/660077/diff/1038/50#newcode351
src/ia32/safe-codegen-ia32.cc:351: // We want to keep the frame, so we
make an unconditional jump look
This seems like a hacky solution. How about giving this code generator
a pair of targets (success and failure) and trying something like this:
VirtualFrame* clone = new VirtualFrame(cgen()->frame());
__ AllocateHeapNumber(...);
Visit(expr);
StoreXMM(final_result()->reg());
scratch()->Unuse();
success.Jump(final_result_);
cgen()->SetFrame(clone);
__ bind(&allocation_failed);
failure.Jump();
http://codereview.chromium.org/660077/diff/1038/50#newcode384
src/ia32/safe-codegen-ia32.cc:384: VirtualFrame* frame = cgen_->frame();
This is:
Result value = cgen()->LoadFromSlot(slot);
http://codereview.chromium.org/660077/diff/1038/50#newcode440
src/ia32/safe-codegen-ia32.cc:440: xmm_stack_height_--;
I don't think it's premature optimization to inline this.
http://codereview.chromium.org/660077/diff/1038/50#newcode445
src/ia32/safe-codegen-ia32.cc:445: xmm_stack_height_++;
Or to inline this. Why not:
return XmmStackElement(0);
? Am I missing something subtle?
http://codereview.chromium.org/660077/diff/1038/50#newcode466
src/ia32/safe-codegen-ia32.cc:466: void SafeGenerator::VisitBlock(Block*
stmt) {
Use the STMT_NODE_LIST macro to generate these, as above.
http://codereview.chromium.org/660077/diff/1038/50#newcode695
src/ia32/safe-codegen-ia32.cc:695: XMMAdd();
Since XMMAdd etc. have only one call site, can't you get rid of them and
write:
XMMRegister left = XMMStackElement(1);
XMMRegister right = XMMStackElement(0);
switch (expr->op()) {
case Token::ADD:
__ addsd(left, right);
break;
case Token::SUB:
__ subsd(left, right);
break;
}
FreeStackTop();
or do you already know that they will show up elsewhere?
http://codereview.chromium.org/660077/diff/1038/49
File src/ia32/safe-codegen-ia32.h (right):
http://codereview.chromium.org/660077/diff/1038/49#newcode43
src/ia32/safe-codegen-ia32.h:43: bool has_supported_syntax() { return
has_supported_syntax_; }
Does this need to be public? Since it doesn't seem to be used from
outside the class (and since has_supported_syntax is not necessarily
use_safe_compiler), it might be safer to have it private.
http://codereview.chromium.org/660077/diff/1038/49#newcode44
src/ia32/safe-codegen-ia32.h:44: bool use_safe_compiler(Expression*
expression) {
The usual naming convention is lowercase_and_underscores for simple
functions like setters and getters (think anything that's constant time
and space). Since this walks an expression tree, it should be named
with CamelCase.
UseSafeCompiler sounds like you might be using the safe compiler here,
not simply checking.
You might consider calling the action of this visitor "Check" so you can
write:
SafeSyntaxChecker checker;
if (checker.Check(expr)) { ... }
http://codereview.chromium.org/660077/diff/1038/49#newcode87
src/ia32/safe-codegen-ia32.h:87: Result VisitAndReturnResult(Expression*
rhs);
Yucky name. How about "Generate". Also, simply call the argument
"expr" or "expression" unless you will actually rely on it specifically
being a rhs of an assignment in some way.
http://codereview.chromium.org/660077/diff/1038/49#newcode115
src/ia32/safe-codegen-ia32.h:115: #define XMMBINOPS(V) \
Yuck to have to use a macro to generate these.
http://codereview.chromium.org/660077
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
