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

Add ISLE rule for heap offset symbol #196

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Add ISLE rule for heap offset symbol #196

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
f5b9c55
Add ISLE rule for heap offset symbol
aborg-dev Jan 23, 2024
fe4edfb
Add comments about offset convention
aborg-dev Jan 26, 2024
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
11 changes: 1 addition & 10 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 @@ -1475,16 +1475,7 @@ impl MachInstEmit for Inst {
ref name,
offset,
} => {
// TODO(akashin): Replace this with a more general logic when we have more than
// one external constant.
let rd = allocs.next_writable(rd);
put_string(
&format!(
"{offset} => {} ;; LoadExtName({name:?})\n",
reg_name(rd.to_reg())
),
sink,
);
unimplemented!("LoadExtName: {name:?}");
}
&Inst::TrapIfC {
rs1,
Expand Down
5 changes: 3 additions & 2 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 @@ -353,9 +353,10 @@
(load_ext_name name 0))

;;;;; Rules for `symbol_value`;;;;;;;;;
;; Heap starts at offset 0 in zkAsm machine memory.
(rule
(lower (symbol_value (symbol_value_data name _ offset)))
(load_ext_name name offset))
(lower (symbol_value (zkasm_base ZkasmBase.Heap)))
(imm $I32 0))
Copy link

Choose a reason for hiding this comment

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

The TODO that was removed from emit.rs mentions having at most one external constant. Hardcoding 0 here means that still at most one external constant is expected?

Copy link
Author

Choose a reason for hiding this comment

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

Not quite. The TODO was out of date, as we support 3 types of external constants now:

wasmtime/cranelift/codegen/src/isa/zkasm/lower/isle.rs

Line 373 in 7605474

fn zkasm_base(&mut self, global_value: GlobalValue) -> Option<generated_code::ZkasmBase> {

But these constants appear in slightly different contexts:

  • For Heap, we use RegOffset addressing mode
  • For Globals, we use Global addressing mode
  • Table is currently not implemented
    I looked for a way to handle the Heap and Globals access similarly, but haven't found it because the generated CLIF code for them is slightly different.

Copy link

Choose a reason for hiding this comment

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

Thank you for the context. I'm struggling to understand why there is a hardcoded 0 on the right hand side of this ISLE rule (instead of any other number)?

Copy link

Choose a reason for hiding this comment

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

Perhaps I should add why I’m struggling: In the previous ISLE rule (that was replaced) name and offset parameters were passed on to load_ext_name. So to see what happens for the symbol_value lowering it was possible to look at how MInst::LoadExtName is handled.

With the new right hand side (imm $I32 0) for the ZkasmBase.Heap case, it’s not clear to me why the hardcoded value is 0 and how this 0 ends up in RegOffset.

Copy link
Author

@aborg-dev aborg-dev Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I see. We use 0 here as the memory region that we use on ZKASM machine starts at offset 0. The offset in global value is populated by us in zkasm.rs Environment

wasmtime/cranelift/wasm/src/environ/zkasm.rs

Line 267 in 7605474

offset: Imm64::new(0),
and is used to identify which memory region we want to access (Heap, Globals, Table). Does this make sense?


;;;;; Rules for `bitcast`;;;;;;;;;
(rule
Expand Down
4 changes: 4 additions & 0 deletions cranelift/codegen/src/isa/zkasm/lower/isle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,10 @@ impl generated_code::Context for ZkAsmIsleContext<'_, '_, MInst, ZkAsmBackend> {
match name {
ExternalName::User(user_name_ref) => {
// All ZKASM "memory"-like accesses use this name, but different offsets.
// This is our convention as is not related to the actual offsets in zkAsm
// machine memory that will be used by each base.
// These offsets need to be aligned with the offsets used in
// `cranelift_wasm::environ::zkasm::ZkasmFuncEnvironment`.
if user_name_ref.index() != 0 {
return None;
}
Expand Down
Loading
Loading