Hi Mihir, Thanks for your contribution! I think you have two good options for followups:
a) Port your switch statement prologue from being in bytecodes to being in Sparkplug and TurboFan (effectively, the first version we almost submitted, except working well with the other compilers). b) As you say, this Smi variable thing. There's no bug for it, I think it's actually quite an original idea from your side. This sort of thing would do well with a design doc first, specifying more precisely what sort of variables you'd be able to inline (my guess would be never-assigned vars with Literal initializers), and what the consequences would be (e.g. the additional memory cost of repeated LdaSmi, smaller stack frames because there's fewer registers, potential for constant folding(?)). I actually have no idea if this would be a net benefit or not, so it'd be good to have an analysis. You could follow the template in our other design docs, e.g. https://docs.google.com/document/d/1qR_C8qYdKsDQFbFliAZLw2zmZ0nhA0xUCld1ba3N2Zk/edit#heading=h.n63k76b3zfwa - Leszek On Wed, Jun 23, 2021 at 5:45 PM Mihir Shah <mihirshah.11...@gmail.com> wrote: > Hi, thank you for all the feedback on the PR, I learnt a lot from the > process! > > Also I was interested in doing another project, I remember you had > mentioned that opportunistic inlining of Smi variables/eliding it entirely > (Commit > message ยท Gerrit Code Review (googlesource.com) > <https://chromium-review.googlesource.com/c/v8/v8/+/2904926/2..20//COMMIT_MSG#b19>) > could be a good future project. > > I was wondering whether you think this would be a good next project for me > to work on, and if so, I was wondering what would be the acceptance > criteria for such a PR (since I can't find an issue on the v8 issue tracker > with a similar purpose)? > > Thank you! > On Monday, May 17, 2021 at 6:35:23 AM UTC-5 les...@chromium.org wrote: > >> I actually meant implementing a Smi check in the bytecode handler itself, >> in interpreter-generator.cc >> <https://source.chromium.org/chromium/chromium/src/+/main:v8/src/interpreter/interpreter-generator.cc;l=2237;drc=cc06b8c77808f6da28919a88c2d7b25fbc327b8b>, >> not as additional bytecodes. >> >> On Mon, May 17, 2021 at 2:55 AM Mihir Shah <mihirsh...@gmail.com> wrote: >> >>> Hi, so I implemented an Smi check by checking (x|0) === x, (since I was >>> not able to get subtracting 0 to work with some of the special case in >>> test/mjstest/switch.js like feeding a NaN or a number very close to an >>> integer case). If the equality check fails, then I jump to the default case >>> or after the switch, depending on whether there is a default case. >>> >>> This passes all the test cases in release mode, but in debug mode, the >>> assertion in the SwitchOnSmi in src/interpreter/interpreter_generator.cc >>> that checks whether the tag of the switch is an Smi fails for certain >>> malformed inputs to the switch (when running the switch.js test). >>> >>> I was not sure how I could get around this, since I did not see any >>> convert to Smi bytecode. >>> >>> Thank you! >>> >>> >>> >>> On Saturday, May 15, 2021 at 11:12:29 AM UTC-5 Mihir Shah wrote: >>> >>>> Oh ok I will add the smi check then, thank you! >>>> >>>> On Saturday, May 15, 2021 at 1:45:42 AM UTC-5 les...@chromium.org >>>> wrote: >>>> >>>>> You could save a register and a TestEqual if you did two jumps (one >>>>> for x<a, one for x>=b) but otherwise there's not much you can do for a >>>>> range check. Keep in mind though that SwitchOnSmi does a range check >>>>> itself >>>>> (and falls through on failure), so unless you want to exclude a range >>>>> that's inside your Switch range, you don't need a range check. >>>>> >>>>> Another thing to keep in mind is that SwitchOnSmi expects a Smi value >>>>> in the accumulator, so you'll need to add a Smi check to the bytecode >>>>> handler. Thankfully 'switch' comparison semantics are that of strict >>>>> equality, so afaict you don't need to add a ToPrimitive or anything like >>>>> that, but I think you might have to add an explicit -0 check before the >>>>> Smi >>>>> check. >>>>> >>>>> Hope that helps, happy to review when you get a patch together - this >>>>> has been on my backlog for literal years, so I'm glad to see someone else >>>>> doing it! >>>>> >>>>> - Leszek >>>>> >>>>> On Sat, 15 May 2021, 07:03 Mihir Shah, <mihirsh...@gmail.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> I was working on a jump table implementation for switch statements >>>>>> with all constant Smi case labels (in the parse step, instead of >>>>>> generating >>>>>> the traditional if-else-if kind of logic), and needed to do range >>>>>> checking >>>>>> at the top. >>>>>> >>>>>> I was wondering, then, was there a better way to do range checking, >>>>>> i.e. does value in accumulator register x lie in range (known at compile >>>>>> time) [a,b]? I think the standard trick of reducing a<=x<=b to >>>>>> 0<=x-a<=b-a >>>>>> and then doing unsigned comparison here would not work because Smi is >>>>>> signed. >>>>>> >>>>>> Because right now my idea of the bytecode is something like this >>>>>> (which feels very inefficient): >>>>>> >>>>>> Load x into accumulator and register r1 ... >>>>>> TestLessThan [b] >>>>>> Star [r2] >>>>>> Ldar [r1] >>>>>> TestGreaterThan [a] >>>>>> TestEqual [r2] >>>>>> JumpIfFalse <after the switch> >>>>>> Ldar [r1] >>>>>> >>>>>> ... proceed with SwitchOnSmi... >>>>>> >>>>>> Thank you! >>>>>> >>>>>> >>>>>> -- >>>>>> -- >>>>>> v8-dev mailing list >>>>>> v8-...@googlegroups.com >>>>>> http://groups.google.com/group/v8-dev >>>>>> --- >>>>>> You received this message because you are subscribed to the Google >>>>>> Groups "v8-dev" group. >>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>> send an email to v8-dev+un...@googlegroups.com. >>>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/d/msgid/v8-dev/6df36377-1d02-4de9-aec4-5890003af416n%40googlegroups.com >>>>>> <https://groups.google.com/d/msgid/v8-dev/6df36377-1d02-4de9-aec4-5890003af416n%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> >>>>> -- >>> -- >>> v8-dev mailing list >>> v8-...@googlegroups.com >>> http://groups.google.com/group/v8-dev >>> --- >>> You received this message because you are subscribed to the Google >>> Groups "v8-dev" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to v8-dev+un...@googlegroups.com. >>> >> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/v8-dev/fd87fb54-8831-4dea-aa22-5045bd892e61n%40googlegroups.com >>> <https://groups.google.com/d/msgid/v8-dev/fd87fb54-8831-4dea-aa22-5045bd892e61n%40googlegroups.com?utm_medium=email&utm_source=footer> >>> . >>> >> -- > -- > v8-dev mailing list > v8-dev@googlegroups.com > http://groups.google.com/group/v8-dev > --- > You received this message because you are subscribed to the Google Groups > "v8-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to v8-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/v8-dev/064ef28d-0577-4541-a94f-8c0332eb0d86n%40googlegroups.com > <https://groups.google.com/d/msgid/v8-dev/064ef28d-0577-4541-a94f-8c0332eb0d86n%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAGRskv_d%2Br0fZifaZwY-o0u1gUouNWEQ53hvswd6gYJ4zw0n-Q%40mail.gmail.com.
