Hi Alvise, On Mon, Jul 4, 2022 at 2:05 AM Alvise <[email protected]> wrote:
> Hi, > > looking at the implementations for signed and unsigned 32-bit divisions in > MachineOperatorReducer, it seems like there's some code that can be > factored together. Do you think it's worth doing it since I'm introducing > the 64-bit versions of these? > > If you think that the code will be more readable and easier to maintain, then it may be worth doing the change. If you are not sure, then it may be better not to change the code. I trust your judgment as a software engineer there. If you submit such a cleanup CL, then it would be good to submit it as a separate CL, e.g. one which just does the refactoring, without adding new functionality, as this will make reviewing the code much easier. If you think about doing multiple cleanups, it would also be good to put each cleanup into a separate CL. > Also, partially related, I saw that the handling of kInt32Mul is slightly > different from the one of kInt32MulWithOverflow: in particular, it looks > like the latter only checks for multiplications by -1 or 2 (which is > transformed into an addition) here > <https://github.com/v8/v8/blob/7a4a6cc6a85650ee91344d0dbd2c53a8fa8dce04/src/compiler/machine-operator-reducer.cc#L391> > and 0 and 1 are handled separately by ReduceProjection here > <https://github.com/v8/v8/blob/7a4a6cc6a85650ee91344d0dbd2c53a8fa8dce04/src/compiler/machine-operator-reducer.cc#L1311>, > while the former handles 0,1,-1, power of twos and consecutive > multiplications by a constant. Could the same set of optimizations be > applied for both? > > I don't think the same optimizations can be done for both multiplications. As the name suggests, kInt32MulWithOverflow has two outputs, first the result of the calculation, and second a boolean flag whether an overflow occurred or not. All transformations of kInt32MulWithOverflow preserve the second output, but the transformation of kInt32Mul for multipliers with powers of two would not preserve the second output. Cheers, Andreas > Hope I'm not being too picky here. > > Have a nice one, > Alvise > > On Wednesday, June 29, 2022 at 1:36:07 PM UTC+2 Andreas Haas wrote: > >> Hi, >> >> The reason could be that this code was written before the addition of >> BigInt to JavaScript, and at that time these reductions were not needed >> because 64-bit divisions were not used. I could imagine that when support >> for WebAssembly and BigInt was added, nobody remembered to add these >> reductions. >> >> I would be happy to review your changes. >> >> Cheers, Andreas >> >> On Tue, Jun 28, 2022 at 2:26 PM Alvise de Faveri Tron <[email protected]> >> wrote: >> >>> Hi all! >>> >>> I was looking at MachineOperatorReducer in src/compiler and I've >>> noticed that, while there are reductions for signed and unsigned 32-bit >>> integer divisions (e.g. >>> MachineOperatorReducer::ReduceInt32Div), there is no such thing for >>> 64-bit divisions. >>> >>> Is there a reason why 64-bit divisions are not optimized? Maybe the *ctz >>> *operation is too slow on 64 bits if there is no intrinsic *ctz* >>> instruction? That's just a wild (and likely incorrect) guess, I'd just be >>> curious to know more. >>> >>> To give some context, I am part of the ongoing effort to port V8 to >>> RISCV32, and we noticed this possibility for optimization in this issue >>> <https://github.com/riscv-collab/v8/issues/574>. >>> >>> If there is no specific reason, I'd be happy to make this contribution. >>> >>> Best to all, >>> - Alvise >>> >>> -- >>> -- >>> 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/8f6d3e85-0141-416c-a82a-ed622b2ff30bn%40googlegroups.com >>> <https://groups.google.com/d/msgid/v8-dev/8f6d3e85-0141-416c-a82a-ed622b2ff30bn%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/74af643d-0b83-467c-b644-85adb789f7efn%40googlegroups.com > <https://groups.google.com/d/msgid/v8-dev/74af643d-0b83-467c-b644-85adb789f7efn%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/CAELSTveHL5nLanBwNm_1onq%3Dw_OYpJkMS%2BtD3dWWOWV0ZtPZyg%40mail.gmail.com.
