Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mul for Imm32 #2

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Mul for Imm32 #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9588014
training changes
MCJOHN974 Sep 18, 2023
a1a42aa
mul for Imm32
MCJOHN974 Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions cranelift/codegen/src/isa/zkasm/inst.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,12 @@
(src1 Imm32)
(src2 Imm32))

;; A multiplication with 2 32-bit immediates.
(MulImm32
(rd WritableReg)
(src1 Imm32)
(src2 Imm32))

))


Expand Down Expand Up @@ -1010,6 +1016,12 @@
(_ Unit (emit (MInst.AddImm32 dst imm1 imm2))))
dst))

(decl zk_mul (Imm32 Imm32) XReg)
(rule (zk_mul imm1 imm2)
(let ((dst WritableXReg (temp_writable_xreg))
(_ Unit (emit (MInst.MulImm32 dst imm1 imm2))))
dst))

;; Helper for emitting the `add` instruction.
;; rd ← rs1 + rs2
(decl rv_add (XReg XReg) XReg)
Expand Down
4 changes: 2 additions & 2 deletions cranelift/codegen/src/isa/zkasm/inst/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,15 +792,15 @@ impl AluOPRRR {
Self::Sllw => "sllw",
Self::Srlw => "srlw",
Self::Sraw => "sraw",
Self::Mul => "mul",
Self::Mul => "MUL",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, looks like ZK ASM does not have MUL operation and we instead would encode this with a combination of call to external environment to do the multiplication and an ARITH instruction to verify the computation as done here: https://github.com/0xPolygonHermez/zkevm-rom/blob/23189df42b058f9a5a0dacbb90b073460816071d/main/utils.zkasm#L638

So I suggest we revert this particular change (args.rs file) for now as it doesn't have any effect and do a separate PR that does general (non-Immediate) multiplication using ARITH by specializing the implementation of &Inst::AluRRR for Mul operation.

Self::Mulh => "mulh",
Self::Mulhsu => "mulhsu",
Self::Mulhu => "mulhu",
Self::Div => "div",
Self::DivU => "divu",
Self::Rem => "rem",
Self::RemU => "remu",
Self::Mulw => "mulw",
Self::Mulw => "MUL",
Self::Divw => "divw",
Self::Divuw => "divuw",
Self::Remw => "remw",
Expand Down
10 changes: 10 additions & 0 deletions cranelift/codegen/src/isa/zkasm/inst/emit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ impl Inst {
| Inst::LoadConst64 { .. }
| Inst::AluRRR { .. }
| Inst::AddImm32 { .. }
| Inst::MulImm32 { .. }
| Inst::FpuRRR { .. }
| Inst::AluRRImm12 { .. }
| Inst::Load { .. }
Expand Down Expand Up @@ -668,6 +669,14 @@ impl MachInstEmit for Inst {
sink,
);
}
&Inst::MulImm32 { rd, src1, src2 } => {
let rd = allocs.next(rd.to_reg());
// TODO(akashin): Should we have a function for `bits` field?
put_string(
&format!("{} * {} => {}\n", src1.bits, src2.bits, reg_name(rd)),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on Monday, we suspect that this only works for a subset of Imm32 values when the overflow does not happen.
I think it is still fine to commit the code as is, but I suggest we add a comment describing this caveat and also introduce a WASM module that tests this and verifies our understanding. Something like:

(module
 (import "env" "assert_eq" (func $assert_eq (param i32) (param i32)))
 (func $main
	i32.const 131072            ;; 2^17
	i32.const 131073            ;; 2^17 + 1
        i32.mul                     ;; This will overflow and wrap.
	i32.const 131072            ;; ((2^17 + 1) * 2^17) % 2^32 = 2^17
	call $assert_eq)
 (start $main))

This will allow us to later validate that the correct implementation handles this case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't implement it fast so will create another PR for other multiplications

sink,
);
}
&Inst::AluRRR {
alu_op,
rd,
Expand Down Expand Up @@ -742,6 +751,7 @@ impl MachInstEmit for Inst {
sink,
);
}

_ => unreachable!("Op {:?} is not implemented", alu_op),
};

Expand Down
9 changes: 9 additions & 0 deletions cranelift/codegen/src/isa/zkasm/inst/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,10 @@ fn zkasm_get_operands<F: Fn(VReg) -> VReg>(inst: &Inst, collector: &mut OperandC
collector.reg_def(*rd);
}

Inst::MulImm32 { rd, src1, src2 } => {
collector.reg_def(*rd);
}

&Inst::VecAluRRRImm5 {
op,
vd,
Expand Down Expand Up @@ -1464,6 +1468,11 @@ impl Inst {
format!("{src1} + {src2} => {rd};")
}

Inst::MulImm32 { rd, src1, src2 } => {
let rd = format_reg(rd.to_reg(), allocs);
format!("{src1} * {src2} => {rd};")
}

&Inst::FpuRR {
frm,
alu_op,
Expand Down
5 changes: 5 additions & 0 deletions cranelift/codegen/src/isa/zkasm/lower.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,11 @@
(rule 5 (lower (has_type (ty_vec_fits_in_register ty) (imul x (splat y))))
(rv_vmul_vx x y (unmasked) ty))


(rule 6 (lower (imul (imm32_from_value x) (imm32_from_value y)))
(zk_mul x y))


;;;; Rules for `smulhi` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(rule 0 (lower (has_type (ty_int_ref_scalar_64 ty) (smulhi x y)))
(lower_smlhi ty (sext x ty $I64) (sext y ty $I64)))
Expand Down
2 changes: 1 addition & 1 deletion cranelift/data/gen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/bin/bash

cargo build
for name in add counter add_func add_memory fibonacci locals locals_simple fibonacci_recursive
for name in add counter add_func add_memory fibonacci locals locals_simple fibonacci_recursive mul
do
echo $name;
../target/debug/clif-util wasm --target sparc-unknown-unknown ../../zkwasm/data/$name.wat > data/$name.zkasm
Expand Down