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 les...@chromium.org 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 <mihirsh...@gmail.com> 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 les...@chromium.org 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, <mihirsh...@gmail.com> 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 >>>>> v8-...@googlegroups.com >>>>> 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 v8-dev+un...@googlegroups.com. >>>>> 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 >> v8-...@googlegroups.com >> 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 v8-dev+un...@googlegroups.com. >> > 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 v8-dev@googlegroups.com 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 v8-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/064ef28d-0577-4541-a94f-8c0332eb0d86n%40googlegroups.com.
