Simple diff for 'Feat/Opcode Circuits for DIV, REM, and REMU opcodes'

[matthiasgoergens]

This is just a simpler view of the diff for https://github.com/scroll-tech/ceno/pull/596/files

[matthiasgoergens]

Just a few comments. It's too long to review in one go.

[matthiasgoergens]

What does the _b suffix stand for?

[bgillesp]

Meant as bool or bit as you prefer -- should I revise the names or add a comment to make this more clear?

[matthiasgoergens]

Using a _bit suffix would be my preferred choice here, as it is still short but leads to less head scratching than just a b.

Thanks.

[matthiasgoergens]

diff --git a/ceno_zkvm/src/instructions/riscv/div.rs b/ceno_zkvm/src/instructions/riscv/div.rs
index 4ed86da4..4d141149 100644
--- a/ceno_zkvm/src/instructions/riscv/div.rs
+++ b/ceno_zkvm/src/instructions/riscv/div.rs
@@ -436,16 +436,10 @@ impl<E: ExtensionField, I: RIVInstruction> Instruction<E> for ArithInstruction<E
                 quotient_signed.assign_instance(instance, lkm, &quotient_v)?;
                 remainder_signed.assign_instance(instance, lkm, &remainder_v)?;
 
-                let remainder_pos_orientation = if negative_division_b {
-                    -(remainder_s as i64)
-                } else {
-                    remainder_s as i64
-                };
-                let divisor_pos_orientation = if divisor_s < 0 {
-                    -(divisor_s as i64)
-                } else {
-                    divisor_s as i64
-                };
+                let neg_if = |b: bool, x: i32| if b { -x } else { x };
+
+                let remainder_pos_orientation = neg_if(negative_division_b, remainder_s) as i64;
+                let divisor_pos_orientation = neg_if(divisor_s < 0, divisor_s) as i64;
 
                 remainder_nonnegative.assign_instance(
                     instance,

This is a bit more compact.

[bgillesp]

Sure, we can write it this way -- note that it's important here that the cast to i64 happens before negation, as one of the issues we run into is the edge case of negating the signed value -2^31.

[matthiasgoergens]

Thanks for thinking about the special case!

I was more thinking about replacing the twice repeated relatively long logic with something shorter to read:

Rust makes it easy to define small helper functions, so we might as well use them.

[matthiasgoergens]

OK, wasn't much left below.

[matthiasgoergens]

All good in this file.

[matthiasgoergens]

See matthias/bg/div-rem-remu-opcodes-suggestions for more suggestions. (Though most of them are already in the review, too.)

[bgillesp]

Thanks Matthias! Gave a look in matthias/bg/div-rem-remu-opcodes-suggestions and didn't find any other changes that haven't been addressed already by various comments. Let me know if you see anything else! 🙏