Hi, Thank you for this feedback, I will go back to the old method then. Also just to follow up, in one of your comment you mentioned, "We could even consider making the remaining conversion code "deferred" so that it doesn't fall out of line." I wasn't understanding what would this look like?
Thank you! On Tuesday, July 6, 2021 at 10:45:01 PM UTC-5 Mihir Shah wrote: > 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/0bbac741-4b9b-4c66-aa69-922cc8e99e49n%40googlegroups.com.
