mozilla :: #ionmonkey

12 Oct 2017
11:14evilpiejandem: does this make sense? https://pastebin.mozilla.org/9069913
11:17jandemevilpie: yes makes sense
11:17jandemtoo bad we can't have more than 2 Values in registers on x86
11:36evilpiejandem: technically the obj, just needs one register
11:36jandemyeah that's true
11:40evilpieah dammit something is still wrong with it
11:43evilpieI guess we need to read the scratchValue out of the frame
11:43evilpie?
11:45jandemevilpie: oh sorry i should have seen that
11:45jandemyeah just leave a value on top of the stack
11:45jandemthat's what the FrameSlot(0) means
11:46jandemcan you mimic SETELEM?
11:47evilpiemhm I don't see how, we don't want to keep rhs on the stack like SETELEM
11:51jandemalternative is to teach the regalloc about the scratch slot
11:51jandembut using TOS somehow would be easier I guess
12:09evilpieI don't get it :/
12:24jandemevilpie: so what's the problem with leaving a value on the stack?
12:24jandemthe fallback stub code can move things around as needed
12:24jandembefore it calls the C++ code
12:25evilpieoh
12:25jandemthat's what setelem does iirc
12:26evilpieok, maybe I can reproduce this
13:07evilpiejandem: success \o/ https://pastebin.mozilla.org/9069917
13:08jandemevilpie: ok but that won't work with the optimized stubs right?
13:08jandemthey expect the value on the stack instead of the scratch slot
13:09evilpieoh :(
13:12evilpiein that case I am out of ideas
13:23bbouvierjandem, ping, re: bug 1406879 do you agree it's safe to land on trunk? (since it's trunk only)
13:23bbouvier(thanks for the review, btw)
13:27jandembbouvier: yeah in that case you don't need sec-approval
13:28jandemevilpie: so.. what if we put the top 2 values in R0 and R1, and leave the 3rd one on the stack?
13:31evilpieso exactly like SETELEM? how would we pop it without getting the stack wrong again after bailing out from ion to baseline
13:41jandemevilpie: i think we have 2 options: either we leave it in the scratch slot and teach the cache regalloc about that - this also assumes the Value is never live across a GC call - that's true I think but not too great (2) in BaselineBailouts.cpp, after builder.popValueInto(PCMappingSlotInfo::SlotInR0), check the *pc and call builder.writeValue(UndefinedValue())
15:16evilpiejandem: thanks for the tip
15:26tcampbellevilpie: thanks for trying to untangle that stuff
15:49evilpietcampbell: jandem: we should think about how we can optimize spread apply and rest arguments
15:52tcampbellyeah, they are very naive right now
15:56evilpieI wonder how hard it would be to use scalar replacement to replace some spread calls to normal calls while allowing inlining
15:56evilpienbp: ^
15:56tcampbelldo we currently do anything for JSOP_FUNAPPLY?
15:56nbpevilpie: how is a spread call different than a normal call?
15:57nbptcampbell: yes, we inline fun-apply calls.
15:57tcampbellevilpie: spreadcall inlining is currently disabled for bailout reasons
15:57evilpieoh I didn't realize we could even do that
15:57nbpevilpie: you mean inlining when we call with the rest arguments array?
15:58evilpieyeah like f(...[1, 2, 3]) to f(1, 2, 3)
15:58evilpieor that would be useful as well I guess
15:58nbptcampbell: what is the issue?
15:58tcampbellnbp: just a lot of bugs
15:58tcampbellnbp: I fixed a bunch of them, but it needs more review
15:59evilpieapparently v8's spread calls are faster than apply
15:59nbptcampbell: do we have a special opcode for spread calls?
15:59evilpiespread calls always seemed more involved to me
15:59tcampbellnbp: yes. there are also a lot of special cases in code for funapply that need roughly similar fixes for spreadcall
16:00nbpevilpie: is f(0, ...arr, 0) valid?
16:01evilpieapparently
16:01evilpiejust tried it
16:02evilpieI am probably going to prepare a nice chart for jandem's promise/call optimization work
16:26jandemevilpie: cool but I have more patches for that though
16:27evilpieI can wait
16:31tcampbellI'm not liking that InitFromBailout needs to check for GETELEM_SUPER =\
16:36tcampbellI wonder if we can unify it with GetStubReturnAddress mechanisms
16:39tcampbellOr maybe enhance BaselineCompiler::emitIC to allow binding the address
16:39tcampbellso we can explictly bind the return address in the baseline code for these tricky cases
17:38evilpietcampbell: sounds good, but this change is really simple though
17:38tcampbellevilpie: perhaps follow up then for a more generral fix
17:40tcampbellevilpie: it just feels like it adds to the general un-maintainability of bailout. too many footguns
17:41evilpieI agree, InitFromBailout is extremely complicated already it seems
17:43tcampbellthings seem to get buried deeper from view
17:43* tcampbell opens a bug
17:58jandemtcampbell: thanks, i agree that would be nicer
13 Oct 2017
No messages
   
Last message: 4 days and 19 hours ago