IR changes for storage accesses #943
Conversation
mohammadfawaz
added
compiler
| match (self.ptr_map.get(&load_val_ptr), self.ptr_map.get(&key_ptr)) { | ||
| (Some(load_val_storage), Some(key_storage)) => { | ||
| match (load_val_storage.clone(), key_storage.clone()) { | ||
| (Storage::Stack(load_val_offset), Storage::Stack(key_offset)) => { |
There was a problem hiding this comment.
Are you able to merge these match statements? Like
(Some(Storage::Stack(load_val_offset)), Some(Storage::Stack(key_offset)))
The load_val_storage and key_storage values don't seem to be used again... I'm surprised Clippy isn't suggesting this, unless I'm missing something.
There was a problem hiding this comment.
Done for all functions.
| comment: "quad state store value".into(), | ||
| owning_span: instr_val.get_span(self.context), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Hrmm, is all of this duplicated from the load value? Is it worth it to have a common method to gather up the pointers and offset and then specialise to SRWQ and SWWQ?
There was a problem hiding this comment.
Done. I wanted to merge the other two functions as well but they're different enough that merging may cause confusion, so I didn't.
| } | ||
| } | ||
| bytes | ||
| } |
There was a problem hiding this comment.
I was looking at this thinking it could be done as a one liner, but the best I could come up with is:
let cnv = |c: char| c.to_digit(16).unwrap() as u8;
s.chunks(2).map(|ch| (cnv(ch[0]) << 4) | cnv(ch[1])).collect::<Vec<_>>().try_into().unwrap()...which I'm not convinced is better. 🙂
There was a problem hiding this comment.
This code is mostly a duplicate of another that you wrote to parser b256 values. I ended up merging the two in one helper function.
| Instruction::StateLoad { load_val, key } => Doc::text_line(format!( | ||
| "state_load ptr {}, key {}{}", | ||
| Instruction::StateLoadQuadWord { load_val, key } => Doc::text_line(format!( | ||
| "state_load_quad_word ptr {}, ptr {}{}", | ||
| namer.name(context, load_val), |
There was a problem hiding this comment.
You've switched the key keyword to be another ptr, which I think might be confusing. When reading the IR it isn't going to be obvious which argument is which. Maybe state_load_quad_word ptr {}, key ptr {}{} is a compromise?
There was a problem hiding this comment.
I like having ptr there to indicate that this is indeed a pointer to a b256, so I now have key ptr {} as per your recommendation.
I've decided to split the storage access work in multiple PRs.
I do have most of the work done for the full flow, but the change is too big to review effectively. This first PR handles IR and IR -> ASM.
Changes in the IR
get_ptrinstruction to also take a pointer type and an offset:get_ptr <base_ptr>, <ptr_ty>, <offset>. The instruction now convertsbase_ptrtoptr_tyand then offsets the result byoffset. The unit ofoffsethere is basically the size ofptr_ty. So, ifptr_tyisptr u64andoffsetis5, then we basically have to add5*sizeof(ptr_ty)tobase_ptr.ptr_castfor now as its functionality can now be accomplished usingget_ptr.state_loadandstate_storewith 4 instructions:state_load_word <key>: returns a word (u64) from the storage locationkey.keyhas to be ab256.state_load_quad_word <val>, <key>: reads 4 words (b256) from storage locationkeyintovalwherevalis a pointer to ab256.state_store_word <val>, <key>: storesvalto storage locationkey.valhas to be au64.state_store_quad_word <val>, <key>: stores 4 words pointed to byvalto storage locationkey.Changes in IR -> ASM
get_ptr.Notes
state_load_*/state_store_*.get_ptrarguments, especiallyloadandstore, but for now, the new arguments are only exercised forstate_load_*andstate_store_*.