github.com/moarvm/moarvm | IRC logs at colabti.org/irclogger/irclogger_logs/moarvm
Set by AlexDaniel on 12 June 2018.
01:03 konvertex left 03:42 farcas1982regreg joined 08:10 konvertex joined 08:20 sena_kun joined
MasterDuke nine: around? 09:29
nine yes 09:30
MasterDuke re github.com/MoarVM/MoarVM/pull/1281...626137192, are you suggesting running MVM_hll_sym_get in spesh's optimize_getcurhllsym and then removing the getcurhllsym and replacing with a getspeshslot instruction? 09:32
similar to what is done with hllbool(for) 09:36
nine Not exactly. If the symbol name passed to getcurhllsym is a contant known value, we can in theory get the hash slot directly in optimize_getcurhllsym and put that into a spesh slot. Then at runtime we only have to fetch that slot, thus get the hash entry and follow its value pointer. 09:39
If the symbol name is not a constant, we can still get the cur HLL's symbol hash, saving at least one hash lookup.
The thing is, that I still can't see where getcurhllsym is used for IO 09:40
DYNAMIC doesn't use it either
MasterDuke uh, i see it when i do a spesh log of `my $o; $o := $*OUT for ^1_000_000;` 09:47
but yeah, it's not used directly by IO, but because the IO is looking up $*OUT 09:48
nine But where?
MasterDuke BB 14 09:49
nine There aren't that many HLL symbols in the first place. It's mostly used for getting the low level ModuleLoader, GLOBAL and @END_PHASERS
MasterDuke 1000009 calls to MVM_hll_sym_get looking up 'GLOBAL' in 'Raku' 09:54
but afk for a bit 09:55
nine Ah, so it is about GLOBAL. Those lookups are done with static strings, which means it could indeed be that perfect case I wrote about
10:16 Altai-man_ joined 10:18 sena_kun left 10:21 mtj_ joined
MasterDuke so an `if (facts->flags & MVM_SPESH_FACT_KNOWN_VALUE)` branch which stores the result of actually calling MVM_hll_sym_get in a spesh slot? and an else branch which does the second part of MVM_hll_sym_get manually? 11:14
to be precise, `if (sym_facts->flags & MVM_SPESH_FACT_KNOWN_VALUE)`? or sym and hll? 11:17
12:07 MasterDuke left
nine Not the result of MVM_hll_sym_get, because that would be the actual value which may change. Instead the result of the MVM_HASH_GET, i.e. the pointer to the hash entry struct which holds the ->value pointer 12:07
This way, it still works when the hash entry gets updated, i.e. a nqp::bindcurhllsym('GLOBAL', $myglobal) 12:08
12:17 sena_kun joined 12:18 Altai-man_ left 12:33 MasterDuke joined
MasterDuke nine: ah, so both branches will manually implement part of MVM_hll_sym_get (just the first branch will do more)? 12:34
nine yes 12:35
MasterDuke ok. i'll give that a shot 12:38
13:54 discord6 joined, discord6 left, discord6 joined
timotimo wasn't there a worry that something in the hll mapping could change that invalidates the lookup we do? 14:01
14:16 Altai-man_ joined 14:18 sena_kun left
MasterDuke i think that's why i'm only going to store a hash entry, not it's value 14:30
though right now i'm getting: `src/jit/x64/emit.dasc:2126: error: bad operand mode in `mov x?,x?': | mov WORK[dst], OBJECT:TMP1->value;` 14:31
nine MasterDuke: TMP1 is no OBJECT 14:40
It's an MVMHashEntry
MasterDuke yeah, i made that change. moarvm now compiles, but i'm getting the occasional segv 14:41
nine timotimo: I'm reasonably sure that my plan is correct, though jnthn may still veto on the grounds that we may not want to fulfil the assumptions I made
timotimo storing a hash entry into a spesh slot sounds like a very bad idea, is that what you're doing? 14:42
MasterDuke yep. seems to work
well, sometimes it works. other times i get a segv or `MoarVM panic: Internal error: invalid thread ID -1071437992 in GC work pass` 14:43
nine Oh, seems like spesh slots are for MVMCollectables only
But....it actually doesn't have to be a spesh slot. Since it's not garbage collected, you may as well use the pointer to the MVMHashEntry as a literal 14:44
It won't change, that's the whole point.
It can only be free'd if the whole HLL disappears, in which case the code that has that pointer baked in would have to be gone, too 14:45
timotimo so is the MVMHashEntry kept around when the hash gets resized? 14:46
nine As well as I understood the macro soup, yes
MasterDuke hm, something doesn't like that change 14:50
timotimo may be easier to introspect the addresses in a live hash in gdb 14:52
MasterDuke this is what i have so far gist.github.com/MasterDuke17/fa730...ee8fa993ef 14:55
but i just realized it shouldn't be sslot in the oplist 14:56
nine No, it's a int64 literal
timotimo don't forget a use_facts 14:57
MasterDuke hm. should it be `ins->operands[1].lit_i64 = entry;` or `sym_facts->value.i = entry`? 15:00
nine I'd say ins->operands[1].lit_i64 15:01
Though they are not mutually exclusive. Both make sense actually 15:02
The code's missing a few error checks (hash and entry != NULL) and needs to mark the thread as blocked before taking the lock 15:04
MasterDuke well either by themselves or both and i still get a segv
i thought we could assume hash would not be null since this wouldn't be the first time we see it? 15:06
nine It's extremely unlikely, but not guaranteed. If there's a language that doesn't use hll syms much and you have hot code like sub i'm-hot() { if $++ > 1000000 { nqp::getcurhllsym("foo") } } then it's possible the getcurhllsym has never been run when we spesh 15:08
MasterDuke ah 15:09
nine Won't happen, but it's just more correct to check
MasterDuke in the asm?
nine No, check in optimize_getcurhllsym and just bail on the optimization if you don't get a hash - it's an unrealistic scenario anyway 15:10
MasterDuke same with entry? 15:15
nine yes
That's actually much more likely to return a negative result 15:16
MasterDuke gist updated. still segvs 15:20
15:21 zakharyas joined
MasterDuke do i need a size on any of my movs? 15:22
oh, think i got it now 15:24
nine what was it?
MasterDuke heh. nope. now it's back to usually working, something a panic
but i changed to `MVMint64 entry = ins->operands[1].lit_i64; | mov64 TMP1, entry;` 15:25
nine You don't need the mov64, plain mov will do the right thing according to the register you use 15:26
But of course, lit_i64 is the right thing
Also make sure no now invalid facts remain 15:27
MasterDuke MVM_spesh_usages_delete_by_reg?
i backed out the mov64, but now it's segv all the time, 15:29
yep, that makes a big difference
nine ??? 15:31
No idea what the rules are then... There are 655 mov in emit.dasc vs. only 14 mov64 15:33
MasterDuke well, just build nqp and rakudo 15:34
nqp tests pass
but a couple fails in spectests 15:39
nine: oh, you mentioned "needs to mark the thread as blocked before taking the lock". MVM_hll_sym_get doesn't do that, but do we need to because we're in a special spesh thread? 15:48
nine The way I see it that's actually a bug in MVM_hll_sym_get. If 2 threads enter that function and the HLL hash doesn't exist and allocating that hash triggers a GC run, we end in a dead lock. 15:50
Because the thread waiting for the lock doesn't let other threads steal it's GC
MasterDuke ah. btw, the segv i'm getting is in my interp.c implementation of sp_getcurhllsym
oh, i removed an operand 15:52
nine Shouldn't that be cur_op += 4;?
15:52 AlexDaniel left
MasterDuke yeah, just made that change. same error though 15:52
nine what causes the segfault? Should be easy to see in gdb
MasterDuke GET_REG(cur_op, 2).i64 is 0 15:53
nine But...you now protect against exactly that? 15:54
MasterDuke yeah...
nine does the spesh log shine any light on this? 15:56
MasterDuke huh. it's not segfaulting if i log... 15:58
it has `sp_getcurhllsym r12(2), liti64(94107760500576) # [000] specialized from getcurhllsym` in the after of DYNAMIC
nine that looks quite ok 16:00
If you run with spesh log in gdb, can you look at that value if it actually points to an MVMHashEntry? 16:07
16:16 sena_kun joined 16:18 Altai-man_ left 16:22 zakharyas left 16:26 zakharyas joined
MasterDuke hm, in gdb with a spesh log it doesn't end up in interp.c 16:27
nine Of course not. That would be too easy now, wouldn't it?
MasterDuke same thing with some of the other test files that failed. segv without a spesh log, no segv with a spesh log 16:29
oh, one did just segv with a spesh log
`(gdb) p *(MVMHashEntry *)93825016168416$2 = {value = 0x555557cef4d8, hash_handle = {tbl = 0x555555ba26f8, hh_next = 0x0, key = 0x555555e9e170, keylen = 0, hashv = 6044822457095861110}}` 16:33
but still segv in interp.c 16:34
nine does *(MVMObject*)((MVMHashEntry *)93825016168416)->value look sensible? 16:35
MasterDuke `(gdb) call MVM_string_utf8_encode_C_string(tc, ((MVMHashEntry *)93825016168416)->hash_handle->key)$4 = 0x555560c4e390 "GLOBAL"`
`$5 = {header = {sc_forward_u = {forwarder = 0x100000016, sc = {sc_idx = 22, idx = 1}, st = 0x100000016}, owner = 1, flags = 17, size = 24}, st = 0x555558208520}`
nine Yep, that's definitely correct 16:36
So...how comes it has a correct value according to the spesh log, but no longer when interpreting the byte code?
MasterDuke so my optimize_getcurhllsym is probably correct, but there's something wrong with my asm? 16:37
nine is the gist up to date? 16:40
Does it work when the JIT is disabled? 16:44
Not sure if it should be .type MVMHASHENTRY, MVMHashEntry or .type MVMHASHENTRY, MVMHashEntry*
MasterDuke updated the gist just to be sure it has exactly what i'm running now 16:46
nine So does it work with JIT disabled? 18:15
18:16 Altai-man_ joined 18:18 sena_kun left 18:22 discord6 left, raku-bridge joined, raku-bridge left, raku-bridge joined
MasterDuke nope 18:48
same segv 18:49
19:08 zakharyas left 19:16 farcas1982regreg left
MasterDuke and still a valid MVMHashEntry pointer in the spesh log 19:27
but GET_REG(cur_op, 2).i64 is 0 19:28
nine Does a call MVM_dump_bytecode(tc) at that position show any light on the issue? 19:29
MasterDuke uh, i think that segfaulted 19:46
yeah
recompiled with optimization off. still segv, but slightly different 19:58
speshlog say pointer is 93825016043664. GET_REG(cur_op, 2).i64 is 137438953554
20:17 sena_kun joined 20:18 Altai-man_ left 20:23 zakharyas joined
MasterDuke is it something about the facts? am i not correctly deleting usage or using facts? 20:30
nine So, no chance of getting a MVM_dump_bytecode? That would tell us what the bytecode really looks like, as the spesh log doesn't seem to tell the full story 20:31
MasterDuke oh, that worked with this unoptimized build
nine can you gist? 20:32
MasterDuke gist.github.com/MasterDuke17/98198...14a3b6d124
nine But, but, but, that still shows the 93825016038512? 20:33
MasterDuke same value as in the spesh log
nine And you have checked *(MVMObject*)((MVMHashEntry *)93825016038512)->value to be sensible
MasterDuke $1 = {header = {sc_forward_u = {forwarder = 0x100000016, sc = {sc_idx = 22, idx = 1}, st = 0x100000016}, owner = 1, flags = 2065, size = 24}, st = 0x55555898cfb0}
nine You haven't deleted usage of the string constant, so spesh didn't delete it. But that won't cause any trouble. 20:34
MasterDuke that's not what `MVM_spesh_usages_delete_by_reg(tc, g, ins->operands[1], ins);` does?
nine Maybe MVM_SPESH_CHECK_DU tells us something 20:35
oh...you did
And GET_REG(cur_op, 2).i64 is NULL in gdb?
MasterDuke $1 = 36426291347713 20:36
if i set MVM_SPESH_CHECK_DU should i expect some output somewhere? 20:39
timotimo only if things are wrong 20:49
MasterDuke ok. there wasn't any output 20:52
nine MasterDuke: what do the other work registers look like? Is the value anywhere to be found? 20:54
20:56 zakharyas left
nine Wait a minute....is the value actually expected to be in a work register at all? How do constants work? 20:56
MasterDuke well, extend_u8 is `GET_REG(cur_op, 0).u64 = (MVMuint64)GET_REG(cur_op, 2).u8` 20:57
nine All ops that take literals do so with code like this: MVMuint64 idx = MVM_BC_get_I64(cur_op, 4);
There's surely a difference between literals and native values in registers 20:58
Does MVM_BC_get_I64(cur_op, 2) yield the value?
MasterDuke yes. yes it does
nine Ah, finally....should have realized this so much sooner :/ 20:59
MasterDuke hm. but i'm still getting a segv. and how there's no backtrace 21:01
jit still disabled 21:02
nine cur_op += 10 21:05
2 bytes for the opcode + 8 bytes for the actual literal value
MasterDuke and how that test file passes 21:06
*now
nine Usually an op consists of the opcode + the 16 bit indices into the WORK array for the arguments, that's why we increment cur_op with some multiple of 2.
MasterDuke time to try nqp+rakudo rebuild and full testing again
nine and a benchmark :)
MasterDuke nine++ i've learned a bunch doing this
nine Oh, does the JIT work as well now?
probably does 21:07
MasterDuke yep
`my $o; $o := $*OUT for ^1_000_000; say $o` takes ~1.14s before, ~0.98s after (rough numbers) 21:08
timotimo wow 21:09
lizmat MasterDuke++ 21:10
nine Nice work! :) 21:17
MasterDuke all tests pass
i still have both `ins->operands[1].lit_i64 = entry;` and `sym_facts->value.i = entry;`. are they both required and/or useful? 21:18
nine I think so 21:19
MasterDuke k 21:20
anything else to do or clean up before pushing a commit? and should i just rebase this over the first commit? 21:21
does this look correct based on the newly changed implementation? `(template: sp_getcurhllsym (^getf (copy $1) MVMHashEntry value))` 22:08
22:16 Altai-man_ joined 22:18 sena_kun left
MasterDuke i'm off to sleep, hopefully will get these last questions cleared up and a finalized PR ready tomorrow 22:39
also, any reason not to do the same thing for gethllsym (assuming MVM_SPESH_FACT_KNOWN_VALUE for both sets of facts)? 22:41
22:54 Kaiepi joined 23:01 Altai-man_ left 23:58 raku-bridge1 joined, raku-bridge1 left, raku-bridge1 joined

Powered by Cro and the Raku® Programming Language.

Please report any issues / comments / feature requests as an issue on App::Raku::Log.

Thank you!