See my inlined comments.
http://codereview.chromium.org/8373029/diff/43002/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/8373029/diff/43002/src/hydrogen.cc#newcode2762 src/hydrogen.cc:2762: iterations = iterations * 2; On 2011/11/07 11:17:04, indutny wrote:
It yields a big improvement, as we usually don't know any type
feedback for some
clauses and therefore can't use any type information here.
We can set clause count limit to 64 to preserve same maximum blocks
count if you
want, but from my benchmarks having two passes of checks and oddball
check
before them is most optimal solution for mutual switch tag's value.
I'm still worried about quadratic size explosion because clause-blocks are not shared between the two passes. In case of double-nested switch-statements you get 4 copies of a clause. We need to share at least the clause bodies (as a result the code for graph building will get even more complicated) My suggestion: First land a simpler version that uses only 1 pass with HStringCompareAndBranch. The graph construction would be much cleaner. Then maybe try to tune it and improve it with concrete benchmark examples. It is _much_ easier to review smaller chunks. The problem with huge changes is that it takes forever to review and turnaround time will be long. http://codereview.chromium.org/8373029/diff/43002/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/8373029/diff/43002/src/ia32/lithium-codegen-ia32.cc#newcode1798 src/ia32/lithium-codegen-ia32.cc:1798: Handle<Code> ic = CompareIC::GetUninitialized(op); On 2011/11/07 11:49:28, indutny wrote:
Just benchmarked it, and found that StringCompareStub is slower for string-to-string comparisons.
Ok, thanks for trying out. Stick to the faster version then. It turns out that CompareIC has a couple of fast checks before going to slow-compare like string length. http://codereview.chromium.org/8373029/diff/50003/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): http://codereview.chromium.org/8373029/diff/50003/src/arm/lithium-arm.cc#newcode1418 src/arm/lithium-arm.cc:1418: return AssignEnvironment(MarkAsCall(DefineFixed(result, r0), instr)); Unintended change? The call to AssignEnvironment seems unnecessary and does not appear on the other platforms. http://codereview.chromium.org/8373029/diff/50003/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/8373029/diff/50003/src/hydrogen.cc#newcode2757 src/hydrogen.cc:2757: return Bailout("SwitchStatement: no symbol comparisons expected"); I don't understand the reason for this bailout. I thought we want to handle symbol- and non-symbol string comparisons? http://codereview.chromium.org/8373029/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
