From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Ostanevich <sergos@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATH luajit] GC64: fix 64-bit constant fusion Date: Mon, 5 Jul 2021 00:06:47 +0300 [thread overview] Message-ID: <20210704210647.GA6106@tarantool.org> (raw) In-Reply-To: <804A99A3-6D0C-4DA9-A939-26FFED0EC823@tarantool.org> Sergos, Thanks for the patch! Something went wrong with formatting the patch I guess, so I'll share you the recipe how I send the backported patches: 1. cherry-pick the original patch from the upstream 2. format-patch the backported commit 3. Add "From" header with the original author of the changes at the very beginning of the formatted patch 4. Adjust the commit message according to our backporting procedure BTW, I've found neither the branch with the patch in tarantool/luajit nor the branch with the green CI in tarantool/tarantool... On 28.05.21, Sergey Ostanevich wrote: > Author: Mike Pall <mike> > Date: Mon Aug 28 10:43:37 2017 +0200 > > x64/LJ_GC64: Fix fallback case of asm_fuseloadk64(). > > Contributed by Peter Cawley. > > (cherry picked from commit 6b0824852677cc12570c20a3211fbfe0e4f0ce14) > > Code generation under LJ_GC64 missed an update to the mcode area after Minor: s/mcode area/mcode area boundaries/. Feel free to ignore. > a 64bit constant encoding. This lead to a corruption to the constant Typo: s/corruption to/corruption of/. > later on. > The problem is rather rare, since there should be big enough (4GiB) > distance from the currently allocated mcode to the dispatch pointer. Minor: Sorry for being pedantic, but this is not such a trivial bug, so I've described semantics around this a bit. As for me it's worth to mention four possible encodings of 64-bit constant on the trace. 0. If the address of the constant fits into 32-bit one, then encode it as a 32-bit displacement (the only option for non-GC64 mode). 1. If the offset of the constant slot from the dispatch table (pinned to r14 that is not changed while trace execution) fits into 32-bit, then encode this as a 32-bit displacement relative to r14. 2. If the offset of the constant slot from the mcode (i.e. rip) fits into 32-bit, then encode this as a 32-bit displacement relative to rip (considering long mode specifics and RID_RIP hack). 3. If none of the conditions above are valid, compiler materializes this 64-bit constant right at the trace bottom and encodes this the same way it does for the previous case. And now goes your part regarding 4Gb and rarity of this case and the problem is much clearer to the reader. Please adjust this part using my description above. > This lead to a number of flaky tests, trackers are addressed. > > Sergey Ostanevich: > * added the description and the test for the problem > > Closes: #4095, #4199, #4614 At first, please move split this line into the separate ones with a single issue per line. Also don't use ':' in GitHub tags, please: it just doesn't respect our commit message style. Unfortunately, this commit tags no issue, since it is pushed to tarantool/luajit repo, but the issues relate to tarantool/tarantool. Hence, you need to explicitly mention the latter repo. And last (but not least), we agreed with Sergey to use "Resolves" instead of "Closes" in tarantool/luajit repo and use "Closes" in the corresponding patches bumping LuaJIT submodule. As for these patches, I'd rather use "Fixes", since you have checked that the failures related to the mentioned issues are gone. Considering everything above, the line above transforms into the following: | Fixes tarantool/tarantool#4095 | Fixes tarantool/tarantool#4199 | Fixes tarantool/tarantool#4614 > > Signed-off-by: Sergey Ostanevich <sergos@tarantool.org> > > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h > index 767bf6f3..2850aea9 100644 > --- a/src/lj_asm_x86.h > +++ b/src/lj_asm_x86.h > @@ -387,6 +387,7 @@ static Reg asm_fuseloadk64(ASMState *as, IRIns *ir) > ir->i = (int32_t)(as->mctop - as->mcbot); > as->mcbot += 8; > as->mclim = as->mcbot + MCLIM_REDZONE; > + lj_mcode_commitbot(as->J, as->mcbot); > } > as->mrm.ofs = (int32_t)mcpofs(as, as->mctop - ir->i); > as->mrm.base = RID_RIP; > diff --git a/test/tarantool-tests/gh-4199-gc64-flaky.test.lua b/test/tarantool-tests/gh-4199-gc64-flaky.test.lua > new file mode 100644 > index 00000000..3ac30427 > --- /dev/null > +++ b/test/tarantool-tests/gh-4199-gc64-flaky.test.lua > @@ -0,0 +1,63 @@ Regarding test code style: I don't want to point each place you have violated it, so I just refer the document[1] that we try to follow except the one rule: we use 2 spaces for indentation to get tests closer to LuaJIT sources. Hence, please consider that I've "implicitly" commented every spot below, where you don't follow our code style. > +-- the test is GC64 only > +local ffi=require('ffi') > +require('utils').skipcond(not ffi.abi('gc64'), 'test is GC64 only') > + > +local tap = require("tap") > +local test = tap.test("gh-4199-gc64-flaky") > +test:plan(1) > + > +-- first - we have to make a gap from current JIT infra to next > +-- available mappable memory > +-- most efficient is to grab it per-page > + > + > +ffi.cdef('void * mmap(void *start, size_t length, int prot , int flags, int fd, long offset);') > +ffi.cdef('long getpagesize();') Minor: It's better use <ffi.cdef> once but with multiline declarations instead of several <ffi.cdef> calls. > + > +local pagesize = tonumber(ffi.C.getpagesize()) > +local blob = {} > +for i=1, 4e9/pagesize do > + blob[i] = ffi.C.mmap(ffi.cast('void*',0), pagesize, 0, 0x22, 0, 0) 0x22, 4e9 -- these are magic numbers for me. Please, create a variable with the descriptive name (and even comments, I guess) for each of them, to ease the further maintenance. > + assert(blob[i] ~= 0) > +end > + > +-- try to chomp all memory in currently allocated gc space > +collectgarbage('stop') Since you decided to stop GC, you can do it right at the very beginning, so you don't need to anchor all mmaped memory above. > +local dummy={'a'} > +for i=2,30 do > + dummy[i] = dummy[i - 1] .. dummy[i - 1] > +end Why do you need this loop? > + > +-- generate a bunch of functions and keep them stored to trigger wrong constant placement > + > +local s={} Again, since GC is stopped, there is no need to anchor all the functions generated below. > +local pass = true > + > +jit.opt.start('hotloop=1’) > +for n=1,100 do > + local src='function f'.. n .. [[(x,y,z,f,g,h,j,k,r,c,d) > + local a={} > + for i=1,1e6 do > + a[i] = x + y + z + f + g + h + j + k + r + c + d > + if (x > 0) then a[i] = a[i] + 1.1 end > + if (c > 0) then a[i] = a[i] + 2.2 end > + if (z > 0) then a[i] = a[i] + 3.3 end > + if (f > 0) then a[i] = a[i] + 4.4 end > + x=x+r > + y=y-c > + z=z+d > + end > + return a[1] > + end > + return f]] .. n ..'(...)' > + > + s[n] = assert(load(src)) > + local res1 = s[n](1,2,3,4,5,6,7,8,9,10,11) > + local res2 = s[n](1,2,3,4,5,6,7,8,9,10,11) This was not obvious to me, but I've finally got it: you compare the result yielded by interpreter with the one yielded by the trace. At first, you don't need to run all 1e6 iterations: you have set 'hotloop' to 1, so you need only 3 (!) iterations to successfully compile the loop. The second call will use the compiled trace for the first loop iteration, but also will try to compile the function itself. AFAIU, the constant being materialized on the trace is the <a> table address, right? At least I see no other option, so please mention in the comment which constant is fused and leads to the failure if the patch is missing. Finally, I don't get, why do you need 100 iterations for getting the misbehaviour. Please, add everything I dumped below as the corresponding comments for this part. > + if (res1 ~= res2) then > + pass = false Why don't you add the assertion about constant fusion right here? > + break > + end > +end > + > +test:ok(pass, 'wrong IR constant fuse') [1]: https://www.tarantool.io/en/doc/latest/dev_guide/lua_style_guide/ -- Best regards, IM
next prev parent reply other threads:[~2021-07-04 21:30 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-28 12:06 Sergey Ostanevich via Tarantool-patches 2021-07-04 21:06 ` Igor Munkin via Tarantool-patches [this message] 2022-02-16 15:44 ` Sergey Kaplun via Tarantool-patches 2022-06-21 12:11 ` sergos via Tarantool-patches 2022-06-22 13:32 ` Sergey Kaplun via Tarantool-patches 2022-06-29 8:04 ` Igor Munkin via Tarantool-patches 2022-06-30 12:10 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210704210647.GA6106@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=sergos@tarantool.org \ --subject='Re: [Tarantool-patches] [PATH luajit] GC64: fix 64-bit constant fusion' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox