Hi, Sorry to bug you again, but I was thinking maybe if we eliminated the extra bytecodes that could potentially reduce optimize::duration, since the optimizer has to handle/optimize on more than a dozen bytecodes extra. I created a design doc for preliminaries on porting (which we were planning on doing later I think anyway), https://docs.google.com/document/d/1Nu5dBnmoL7IcFI-ZPvtjhk97rqu8B6m9IgQgQXLgPl4/edit?resourcekey=0-L-yAxQwVqRlKtIXmomJ1-g, can you please let me know if this sounds good?
Thank you! On Monday, July 5, 2021 at 6:01:55 AM UTC-5 [email protected] wrote: > (v8-dev to bcc) > > I replied on the bug, basically I imagine that Switch nodes aren't well > supported by the optimizer -- either because of node traversal costs, or > because of optimization passes being designed only for Branch. > > On Wed, Jun 30, 2021 at 5:30 PM Mihir Shah <[email protected]> wrote: > >> 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 >> >> <https://groups.google.com/d/msgid/v8-dev/ac4a9ade-2889-4548-bb50-d2fa723e580an%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/ff29a181-2d84-4c99-99d7-25fc19e6b15en%40googlegroups.com.
