NOT FOR COMMITTING: Add fast side-effect free floating point expressions.

Add fast side-effect free floating point expressions.  This will not be committed in this form, so it does not need LGTM approval.

[William Hesse, 2010-02-25 11:50:06]

Here are two skeletal files to be filled in with the implementation of safe
expressions.

[Lasse Reichstein, 2010-02-25 12:00:21]

LGTM - as a work in progress.

http://codereview.chromium.org/660077/diff/1002/1003
File src/ia32/safe-codegen-ia32.h (right):

http://codereview.chromium.org/660077/diff/1002/1003#newcode78
src/ia32/safe-codegen-ia32.h:78: int xmm_stack_height_;
Is this just for XMM register allocation? If so, does it need to be a stack?

[Kevin Millikin (Chromium), 2010-02-28 10:20:29]

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.

[William Hesse, 2010-03-01 16:05:06]

All suggested changes made, except that LoadFromSlot is a private method of
CodeGenerator.  Note that I am not planning to commit this change - it is
experimental.