[turbofan] Why no reductions for 64-bit divisions?

[Alvise de Faveri Tron]

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.

If there is no specific reason, I'd be happy to make this contribution.

Best to all,

- Alvise

[Andreas Haas]

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

--
--
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/8f6d3e85-0141-416c-a82a-ed622b2ff30bn%40googlegroups.com.

[Alvise]

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?

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 and 0 and 1 are handled separately by ReduceProjection here, 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?

Hope I'm not being too picky here.

Have a nice one,

Alvise

[Andreas Haas]

Hi Alvise,

On Mon, Jul 4, 2022 at 2:05 AM Alvise <elvi...@gmail.com> 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 and 0 and 1 are handled separately by ReduceProjection here, 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

 

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/74af643d-0b83-467c-b644-85adb789f7efn%40googlegroups.com.

[Alvise]

Hi Andreas,

Thanks for the clarification.

Cheers,

Alvise