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
