Hi, Also I had another question, I noticed in the TryTaggedToInt32AsIntPtr() method, in the case it is not an Smi but is a Heap number, we load the float64, truncate to an int32, and then do the strict equality check between the original and truncated version to make sure the new form is equivalent.
I was wondering, in what case would the float64 ever be equivalent to an int32, except for -0.0? (Because I remember like we were saying that any float representable as an Smi will be stored as an Smi, back when we were doing the switch case analysis) In this case, I was wondering if we could streamline the logic just to check for a -0.0? Thank you! On Wednesday, July 7, 2021 at 10:54:53 AM UTC-5 [email protected] wrote: > v8-dev to bcc > > It's easier to discuss this further in the doc, but basically in CSA code > you can mark a rarely-taken label as "deferred" to move that chunk of code > to the end of the generated code. This makes the fast-path of not taking > the label faster. > > On Wed, Jul 7, 2021 at 4:43 PM Mihir Shah <[email protected]> wrote: > >> 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 >> >> <https://groups.google.com/d/msgid/v8-dev/0bbac741-4b9b-4c66-aa69-922cc8e99e49n%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/67731bdf-0451-4006-9b90-36cac7886c50n%40googlegroups.com.
