Here are my comments: I'd like to see the code simpler if possible. Also lazy-deoptimization is a major concern.
http://codereview.chromium.org/8373029/diff/32026/src/ast.h File src/ast.h (right): http://codereview.chromium.org/8373029/diff/32026/src/ast.h#newcode713 src/ast.h:713: bool IsSymbolCompare() { return compare_type_ == SYMBOL_ONLY; } You introduce IsSymbolCompare, but it does not seem to be used anywhere. Can you use it in the same way we use the type feedback for smis? http://codereview.chromium.org/8373029/diff/32026/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/8373029/diff/32026/src/hydrogen.cc#newcode2786 src/hydrogen.cc:2786: compare = new(zone()) HCompareGenericAndBranch(context, tag_value, Not sure this works: If you pass an object as tag value, we may get a lazy-deoptimization at this instruction: The generic compare may invoke other JS code through valueOf method. The problem is that we can't insert a LLazyBailout instruction for the lazy-deopt after a branch. That's why we only had CompareGeneric (without branch). How about only handling the string-cases, and deoptimize on others? I think the code would get a lot simpler, too. http://codereview.chromium.org/8373029/diff/32026/src/hydrogen.cc#newcode2804 src/hydrogen.cc:2804: if (last_block != NULL) { Can it happen at all that we deoptimize (and last_block == NULL) if there are only string-literals? last_block should be never NULL in this case, shouldn't it? http://codereview.chromium.org/8373029/diff/32026/src/ia32/lithium-ia32.cc File src/ia32/lithium-ia32.cc (right): http://codereview.chromium.org/8373029/diff/32026/src/ia32/lithium-ia32.cc#newcode1464 src/ia32/lithium-ia32.cc:1464: return AssignEnvironment(MarkAsCall(DefineFixed(result, eax), instr)); MarkAsCall should already assign environments if necessary, so no need to explicitly do it here. http://codereview.chromium.org/8373029/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
