Hi Andreas, Thanks for the clarification.
Cheers, Alvise On Monday, July 4, 2022 at 9:28:56 AM UTC+2 [email protected] wrote: > 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/92e48d07-972e-498c-8474-554487f2a712n%40googlegroups.com.
