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.

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

Finish M extension #434

Merged
merged 23 commits into from
Aug 9, 2024
Merged

Finish M extension #434

merged 23 commits into from
Aug 9, 2024

Conversation

moodlezoup
Copy link
Collaborator

@moodlezoup moodlezoup commented Jul 26, 2024

Finishes wiring up the M extension to Jolt:

  • tracer support h/t @ncitron
  • Adds new constraints
  • Fixes various bugs (described in-line)

Comment on lines +361 to +367
// All instructions in virtual sequence are mapped from the same
// ELF address. Thus if an instruction is virtual (and not the last one
// in its sequence), then we should *not* update the PC.
flags[11] = match self.virtual_sequence_remaining {
Some(i) => i != 0,
None => false
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need this flag (and so we had to reverse the virtual sequence indexing) for the new non-uniform constraint, which ensures that virtual sequences are executed in full and in order

@@ -26,6 +26,23 @@ impl JoltField for ark_bn254::Fr {
}
}

fn to_u64(&self) -> Option<u64> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for debugging

@@ -190,7 +201,26 @@ impl<const WORD_SIZE: usize> VirtualInstructionSequence for DIVInstruction<WORD_
advice_value: None,
});

virtual_sequence
virtual_trace.push(RVTraceRow {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, we had elided the MOVE instruction as described in the Jolt paper as an optimization, instead writing the advice directly to the destination register
But as it turns out, we actually did need the separate virtual register + MOVE for the edge case where rd is equal to rs1 or rs2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was implemented as a signed ≤, but it should be an unsigned ≤

@moodlezoup moodlezoup requested a review from sragss July 26, 2024 20:18
@moodlezoup moodlezoup marked this pull request as ready for review July 26, 2024 20:18
Copy link
Collaborator

@sragss sragss left a comment

Choose a reason for hiding this comment

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

LGTM

jolt-core/src/jolt/vm/bytecode.rs Show resolved Hide resolved
jolt-core/src/jolt/vm/bytecode.rs Outdated Show resolved Hide resolved
SubtableIndices::from(C - 1),
),
(
// Not used for lookup, but this implicitly range-checks
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make a utility function for this so it's self documenting and use across all the memory ops that need range-checking.

fn RangeCheck(C) -> (Box<Subtable<...>>, SubtableIndices)

@moodlezoup moodlezoup merged commit 5cff005 into main Aug 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants