Hi, sorry to bug you but just to follow up, I was not understanding why creating a chain of branches would improve upon a large switch node ( 1223133 - [V8 Perf Sheriff]: 3 regressions in v8.browsing_desktop - chromium <https://bugs.chromium.org/p/chromium/issues/detail?id=1223133#c9>), in improving the optimize duration performance? Thank you! On Thursday, June 24, 2021 at 5:35:53 AM UTC-5 [email protected] wrote:
> 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 <[email protected]> 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 [email protected] 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 <[email protected]> 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 [email protected] >>>>> 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, <[email protected]> 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 >>>>>>> [email protected] >>>>>>> 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 [email protected]. >>>>>>> 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 >>>> [email protected] >>>> 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 [email protected]. >>>> >>> 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 >> [email protected] >> 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 [email protected]. >> > 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 [email protected] 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/ac4a9ade-2889-4548-bb50-d2fa723e580an%40googlegroups.com.
