Hi, So I was implementing the baseline compiler routine to do the Smi conversion, and I just wanted to make sure I was on the right track: Here is a short document outlining what I am doing, whenever it is possible can you please look through this? https://docs.google.com/document/d/1hev0vI7o7gjD85fUkZVYcg3rt0guZ_bk6lNkHsSkt-w/edit?usp=sharing
Thank you! On Friday, July 9, 2021 at 3:10:16 AM UTC-5 [email protected] wrote: > There's almost certainly cases in the runtime where we don't normalize a > HeapNumber to a Smi -- my comment on floats representable as Smis being > normalized to Smis was only for the numbers generated by the parser. > > On Thu, Jul 8, 2021 at 9:20 PM Mihir Shah <[email protected]> wrote: > >> 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 >> >> <https://groups.google.com/d/msgid/v8-dev/67731bdf-0451-4006-9b90-36cac7886c50n%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/bf3fbea3-72ef-44d5-98e5-615ff335f6bdn%40googlegroups.com.
