I have updated the fix but I'm not a v8 committer. On Tue, Jun 14, 2011 at 1:40 AM, Kevin Millikin <[email protected]>wrote:
> Hi Stephen, > > OK, I agree that's a silly jump. You're right that folding in the parser > is simplest here (it complicates the compiler a lot to test for and handle > the unreachable labels after every branch, which is why we don't already > eliminate the dead code). > > There is no documentation of the Crankshaft intermediate languages > (Hydrogen and Lithium) other than the code itself. They are still changing. > > > On Fri, Jun 10, 2011 at 8:52 PM, Stephen Adams <[email protected]> wrote: > >> >> >> On Fri, Jun 10, 2011 at 4:26 AM, Kevin Millikin <[email protected]> >> wrote: >> > Stephen, we also have this same constant folding in the >> hydrogen->lithium >> > translation, with the drawback that it generates dead code for the >> not-taken >> > branch and a jump around it on one side or the other. I'd be interested >> in >> > knowing if it's this dead code and jump you want to get rid of, or if we >> > have a case where that's really not working for us and we're testing the >> > value of the constant. >> >> I wanted to get rid of the jump over the dead code. In the unoptimized >> version (I expect most code stays in that state) I see two unnecessary jumps >> and two unused embedded objects. >> >> I looked at a few scripts from a large app and there are hundreds of >> occurrences of !0 and !1. >> The one I looked at in detail, half of the occurrences of 0 and 1 were in >> !0 and !1. >> The dead code must exert some memory pressure and it contains embedded >> <true> or <false> objects, so there is more overhead than just the >> instruction bytes. >> >> Constant folding early should improve throughput at all levels of the >> system. I don't think constant folding here it is particularly evil, since >> the only sane reason to write !0 is as a shorthand for 'true'. >> >> Is there a document that would tell me what hydrogen and lithium are? >> >> From my previous email to Kasper: >> >> --- Raw source --- >> (n) { >> g = ! 0; >> } >> >> >> --- Code --- >> kind = FUNCTION >> name = testB0 >> Instructions (size = 76) >> 0xf2c02a20 0 55 push ebp >> 0xf2c02a21 1 89e5 mov ebp,esp >> 0xf2c02a23 3 56 push esi >> 0xf2c02a24 4 57 push edi >> 0xf2c02a25 5 3b2500a5930c cmp esp,[0xc93a500] >> 0xf2c02a2b 11 7305 jnc 18 (0xf2c02a32) >> 0xf2c02a2d 13 e88e5afeff call 0xf2be84c0 ;; debug: >> statement 157 >> ;; code: STUB, >> StackCheck, minor: 0 >> 0xf2c02a32 18 eb10 jmp 36 (0xf2c02a44) >> 0xf2c02a34 20 3da580c0f2 cmp eax, 0xf2c080a5 ;; object: >> 0xf2c080a5 <true> >> 0xf2c02a39 25 0f840c000000 jz 43 (0xf2c02a4b) >> 0xf2c02a3f 31 e900000000 jmp 36 (0xf2c02a44) >> 0xf2c02a44 36 b8a580c0f2 mov eax,0xf2c080a5 ;; object: >> 0xf2c080a5 <true> >> 0xf2c02a49 41 eb05 jmp 48 (0xf2c02a50) >> 0xf2c02a4b 43 b8c580c0f2 mov eax,0xf2c080c5 ;; object: >> 0xf2c080c5 <false> >> 0xf2c02a50 48 b951c3c0f2 mov ecx,0xf2c0c351 ;; object: >> 0xf2c0c351 <String[1]: g> >> 0xf2c02a55 53 8b5617 mov edx,[esi+23] >> 0xf2c02a58 56 e803b9feff call StoreIC_Initialize (0xf2bee360) >> ;; debug: statement 165 >> ;; debug: >> position 167 >> ;; code: >> contextual, STORE_IC, UNINITIALIZED >> 0xf2c02a5d 61 b85180c0f2 mov eax,0xf2c08051 ;; object: >> 0xf2c08051 <undefined> >> 0xf2c02a62 66 89ec mov esp,ebp ;; debug: >> position 174 >> ;; js return >> 0xf2c02a64 68 5d pop ebp >> 0xf2c02a65 69 c20800 ret 0x8 >> >> Stack checks (size = 0) >> ast_id pc_offset >> >> RelocInfo (size = 23) >> 0xf2c02a2d statement position (157) >> 0xf2c02a2e code target (STUB) (0xf2be84c0) >> 0xf2c02a35 embedded object (0xf2c080a5 <true>) >> 0xf2c02a45 embedded object (0xf2c080a5 <true>) >> 0xf2c02a4c embedded object (0xf2c080c5 <false>) >> 0xf2c02a51 embedded object (0xf2c0c351 <String[1]: g>) >> 0xf2c02a58 statement position (165) >> 0xf2c02a58 position (167) >> 0xf2c02a59 code target (context) (STORE_IC) (0xf2bee360) >> 0xf2c02a5e embedded object (0xf2c08051 <undefined>) >> 0xf2c02a62 position (174) >> 0xf2c02a62 js return >> >> >> --- Raw source --- >> (n) { >> g = ! 0; >> } >> >> >> --- Optimized code --- >> kind = OPTIMIZED_FUNCTION >> name = testB0 >> stack_slots = 0 >> Instructions (size = 85) >> 0xf2c70aa0 0 55 push ebp >> 0xf2c70aa1 1 89e5 mov ebp,esp >> 0xf2c70aa3 3 56 push esi >> 0xf2c70aa4 4 57 push edi >> 0xf2c70aa5 5 8b45fc mov eax,[ebp-4] >> 0xf2c70aa8 8 3b2500b5bb0c cmp esp,[0xcbbb500] >> 0xf2c70aae 14 7308 jnc 24 (0xf2c70ab8) >> 0xf2c70ab0 16 8b75fc mov esi,[ebp-4] >> 0xf2c70ab3 19 e8085afeff call 0xf2c564c0 ;; code: >> STUB, StackCheck, minor: 0 >> 0xf2c70ab8 24 e90b000000 jmp 40 (0xf2c70ac8) >> 0xf2c70abd 29 c7c0c560c7f2 mov eax,0xf2c760c5 ;; object: >> 0xf2c760c5 <false> >> 0xf2c70ac3 35 e906000000 jmp 46 (0xf2c70ace) >> 0xf2c70ac8 40 c7c0a560c7f2 mov eax,0xf2c760a5 ;; object: >> 0xf2c760a5 <true> >> 0xf2c70ace 46 8905c46fc1f2 mov [0xf2c16fc4],eax ;; global >> property cell >> 0xf2c70ad4 52 b85160c7f2 mov eax,0xf2c76051 ;; object: >> 0xf2c76051 <undefined> >> 0xf2c70ad9 57 89ec mov esp,ebp >> 0xf2c70adb 59 5d pop ebp >> 0xf2c70adc 60 c20800 ret 0x8 >> 0xf2c70adf 63 90 nop >> 0xf2c70ae0 64 90 nop >> 0xf2c70ae1 65 90 nop >> 0xf2c70ae2 66 90 nop >> 0xf2c70ae3 67 90 nop >> >> Safepoints (size = 17) >> 0xf2c70ab8 24 11111111 (sp -> fp) 0 >> >> RelocInfo (size = 6) >> 0xf2c70ab4 code target (STUB) (0xf2c564c0) >> 0xf2c70abf embedded object (0xf2c760c5 <false>) >> 0xf2c70aca embedded object (0xf2c760a5 <true>) >> 0xf2c70ad0 global property cell >> 0xf2c70ad5 embedded object (0xf2c76051 <undefined>) >> >> > >> > On Fri, Jun 10, 2011 at 12:05 PM, <[email protected]> wrote: >> >> >> >> http://codereview.chromium.org/7044100/diff/1/src/parser.cc >> >> File src/parser.cc (right): >> >> >> >> >> http://codereview.chromium.org/7044100/diff/1/src/parser.cc#newcode2559 >> >> src/parser.cc:2559: if (value == 0) { >> >> On 2011/06/10 09:20:08, Kasper Lund wrote: >> >>> >> >>> Could this be something ala: >> >> >> >>> Handle<Object> boolean(isolate->heap()->ToBoolean(value == 0)); >> >>> return new(zone()) Literal(boolean); >> >> >> >>> or perhaps consider negating the result of calling >> >>> expression->AsLiteral()->handle()->ToBoolean() and refactoring the >> >> >> >> code so it >> >>> >> >>> works for cases where the expression is a non-numeric literal. >> >> >> >> We already have this constant folding in void >> >> FullCodeGenerator::TestContext::Plug(Handle<Object> lit). It just >> needs >> >> to be 'ported' to TestContext::BuildBranch(HValue* value) in >> >> hydrogen.cc. >> >> >> >> I would prefer that solution rather than having it here, because it can >> >> properly cope with things like >> >> >> >> x = 0 >> >> if (!x) ... >> >> >> >> http://codereview.chromium.org/7044100/ >> > >> > >> >> > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
