(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/CAGRskv-cSa%2BqyhbX9KD6M804qZBX0156QvTBw-Bf8HSRgKPScA%40mail.gmail.com.
