Superblocks #3502
Superblocks #3502
Conversation
…into fallible-execution-api
autoprecompiles/src/adapter.rs
Outdated
| pub type AdapterBasicBlock<A> = BasicBlock<<A as Adapter>::Instruction>; | ||
| pub type AdapterBasicBlock<A> = Block<<A as Adapter>::Instruction>; |
There was a problem hiding this comment.
Can we call it AdapterBlock here? Was a bit confused a few times as I was expecting this to be basic block but it's actually not.
There was a problem hiding this comment.
made BasicBlock a separate type again
There was a problem hiding this comment.
So in Instruction mode, it doesn't seem that we have the "select candidates" process that accounts for overlapping super blocks as in Cell. I wonder if this was intentional.
There was a problem hiding this comment.
good point, was mostly paying attention to cell pgo, but indeed I think we should take that into account (I'll add a TODO for now)
| // by the solver. | ||
| let pc = (i == 0).then_some(block.start_pc); | ||
| let pc = if i == 0 { | ||
| Some(block.start_pc) | ||
| } else { | ||
| block | ||
| .other_pcs | ||
| .iter() | ||
| .find(|(idx, _)| *idx == i) | ||
| .map(|(_, pc_value)| *pc_value) | ||
| }; | ||
| let pc_lookup_row = instr |
There was a problem hiding this comment.
So these seem to create pc lookup constraints for the start pc of each basic block within the super block?
I wonder how is this different from the optimistic constraints for pc created else where.
There was a problem hiding this comment.
i think these are two different things:
- at execution we need to check that an APC can be used at that given point (optimistic constraint)
- these here are real constraints, the prover needs to enforce the jumps when a given superblock is selected
|
As an entirely separate question and maybe not this PR, but how do we select which APC to execute, for example, when we have both Is it to execute the "largest" possible? I couldn't seem to locate an execution logic some where. |
also add a comment regarding overlapping block discounts
It should pick the "best" according to cell PGO, that is |
qwang98
force-pushed
the
superblocks-integrate
branch
from
ce8a789 to
5d07fb9
Compare
|
FYI I incorrectly pushed two commits to this branch, which I have restored via force push, so we should still be at the prior latest commit. |
| #[derive(Debug, Serialize, Deserialize, Clone)] | ||
| pub enum Block<I> { | ||
| Basic(BasicBlock<I>), | ||
| Super(SuperBlock<I>), | ||
| } |
There was a problem hiding this comment.
I'm not sure about this because there are two ways to express a single basic block b:
Block::Basic(b) and Block::Super(SuperBlock { blocks: vec![b] })
I suspect we should rather rename SuperBlock to Block and use it everywhere. Then a normal block is an edge case of a superblock, and there's only one way to represent it: Block { basic_blocks: vec![b] }
Is there another reason why we would need an enum here?
There was a problem hiding this comment.
I checked again: superblock collection used to be Vec<Block> -> Vec<Block>, now it's Vec<BasicBlock> -> Vec<BasicBlock or SuperBlock> but it seems like it should be Vec<BasicBlock> -> Vec<SuperBlock>
[superblocks] remove block boundary pc constraint
Superblock detection and selection from PGO