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

chore(avm-simulator): revive field comparison #4957

Merged
merged 1 commit into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions yarn-project/simulator/src/avm/avm_memory_types.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,13 @@ describe('Field', () => {
expect(field1.equals(field3)).toBe(false);
});

it(`Should check if one Field is less than another correctly`, () => {
const field1 = new Field(5);
const field2 = new Field(10);
expect(field1.lt(field2)).toBe(true);
expect(field2.lt(field1)).toBe(false);
});

it(`Should convert Field to BigInt correctly`, () => {
const field = new Field(5);
expect(field.toBigInt()).toStrictEqual(5n);
Expand Down
7 changes: 5 additions & 2 deletions yarn-project/simulator/src/avm/avm_memory_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export abstract class MemoryValue {
public abstract div(rhs: MemoryValue): MemoryValue;

public abstract equals(rhs: MemoryValue): boolean;
public abstract lt(rhs: MemoryValue): boolean;

// We need this to be able to build an instance of the subclasses.
public abstract build(n: bigint): MemoryValue;
Expand All @@ -37,8 +38,6 @@ export abstract class IntegralValue extends MemoryValue {
public abstract or(rhs: IntegralValue): IntegralValue;
public abstract xor(rhs: IntegralValue): IntegralValue;
public abstract not(): IntegralValue;

public abstract lt(rhs: IntegralValue): boolean;
}

/**
Expand Down Expand Up @@ -164,6 +163,10 @@ export class Field extends MemoryValue {
return this.rep.equals(rhs.rep);
}

public lt(rhs: Field): boolean {
return this.rep.lt(rhs.rep);
}

public toBigInt(): bigint {
return this.rep.toBigInt();
}
Expand Down
31 changes: 23 additions & 8 deletions yarn-project/simulator/src/avm/opcodes/comparators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,19 @@ describe('Comparators', () => {
expect(actual).toEqual([new Uint32(0), new Uint32(1), new Uint32(0)]);
});

it('Does not work on field elements', async () => {
await expect(() =>
new Lt(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 1, /*dstOffset=*/ 10).execute(context),
).rejects.toThrow(TagCheckError);
it('Works on field elements', async () => {
context.machineState.memory.setSlice(0, [new Field(1), new Field(2), new Field(0)]);

[
new Lt(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 0, /*dstOffset=*/ 10),
new Lt(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 1, /*dstOffset=*/ 11),
new Lt(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 2, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
expect(actual).toEqual([new Field(0), new Field(1), new Field(0)]);
});

it('InTag is checked', async () => {
context.machineState.memory.setSlice(0, [new Field(1), new Uint32(2), new Uint16(3)]);

Expand Down Expand Up @@ -166,10 +174,17 @@ describe('Comparators', () => {
expect(actual).toEqual([new Uint32(1), new Uint32(1), new Uint32(0)]);
});

it('Does not work on field elements', async () => {
await expect(() =>
new Lte(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 1, /*dstOffset=*/ 10).execute(context),
).rejects.toThrow(TagCheckError);
it('Works on field elements', async () => {
context.machineState.memory.setSlice(0, [new Field(1), new Field(2), new Field(0)]);

[
new Lte(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 0, /*dstOffset=*/ 10),
new Lte(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 1, /*dstOffset=*/ 11),
new Lte(/*indirect=*/ 0, TypeTag.FIELD, /*aOffset=*/ 0, /*bOffset=*/ 2, /*dstOffset=*/ 12),
].forEach(i => i.execute(context));

const actual = context.machineState.memory.getSlice(/*offset=*/ 10, /*size=*/ 4);
expect(actual).toEqual([new Field(1), new Field(1), new Field(0)]);
});

it('InTag is checked', async () => {
Expand Down
11 changes: 4 additions & 7 deletions yarn-project/simulator/src/avm/opcodes/comparators.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { AvmContext } from '../avm_context.js';
import { IntegralValue, TaggedMemory } from '../avm_memory_types.js';
import { Opcode } from '../serialization/instruction_serialization.js';
import { ThreeOperandInstruction } from './instruction_impl.js';

Expand Down Expand Up @@ -35,10 +34,9 @@ export class Lt extends ThreeOperandInstruction {

async execute(context: AvmContext): Promise<void> {
context.machineState.memory.checkTags(this.inTag, this.aOffset, this.bOffset);
TaggedMemory.checkIsIntegralTag(this.inTag);

const a = context.machineState.memory.getAs<IntegralValue>(this.aOffset);
const b = context.machineState.memory.getAs<IntegralValue>(this.bOffset);
const a = context.machineState.memory.get(this.aOffset);
const b = context.machineState.memory.get(this.bOffset);

// Result will be of the same type as 'a'.
const dest = a.build(a.lt(b) ? 1n : 0n);
Expand All @@ -58,10 +56,9 @@ export class Lte extends ThreeOperandInstruction {

async execute(context: AvmContext): Promise<void> {
context.machineState.memory.checkTags(this.inTag, this.aOffset, this.bOffset);
TaggedMemory.checkIsIntegralTag(this.inTag);

const a = context.machineState.memory.getAs<IntegralValue>(this.aOffset);
const b = context.machineState.memory.getAs<IntegralValue>(this.bOffset);
const a = context.machineState.memory.get(this.aOffset);
const b = context.machineState.memory.get(this.bOffset);

// Result will be of the same type as 'a'.
const dest = a.build(a.equals(b) || a.lt(b) ? 1n : 0n);
Expand Down
4 changes: 2 additions & 2 deletions yellow-paper/docs/public-vm/gen/_instruction-set.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ Less-than check (a < b)
- **Category**: Compute - Comparators
- **Flags**:
- **indirect**: Toggles whether each memory-offset argument is an indirect offset. Rightmost bit corresponds to 0th offset arg, etc. Indirect offsets result in memory accesses like `M[M[offset]]` instead of the more standard `M[offset]`.
- **inTag**: The [tag/size](./memory-model#tags-and-tagged-memory) to check inputs against and tag the destination with. `field` type is NOT supported for this instruction.
- **inTag**: The [tag/size](./memory-model#tags-and-tagged-memory) to check inputs against and tag the destination with.
- **Args**:
- **aOffset**: memory offset of the operation's left input
- **bOffset**: memory offset of the operation's right input
Expand All @@ -612,7 +612,7 @@ Less-than-or-equals check (a <= b)
- **Category**: Compute - Comparators
- **Flags**:
- **indirect**: Toggles whether each memory-offset argument is an indirect offset. Rightmost bit corresponds to 0th offset arg, etc. Indirect offsets result in memory accesses like `M[M[offset]]` instead of the more standard `M[offset]`.
- **inTag**: The [tag/size](./memory-model#tags-and-tagged-memory) to check inputs against and tag the destination with. `field` type is NOT supported for this instruction.
- **inTag**: The [tag/size](./memory-model#tags-and-tagged-memory) to check inputs against and tag the destination with.
- **Args**:
- **aOffset**: memory offset of the operation's left input
- **bOffset**: memory offset of the operation's right input
Expand Down
4 changes: 2 additions & 2 deletions yellow-paper/src/preprocess/InstructionSet/InstructionSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ const INSTRUCTION_SET_RAW = [
"Category": "Compute - Comparators",
"Flags": [
{"name": "indirect", "description": INDIRECT_FLAG_DESCRIPTION},
{"name": "inTag", "description": IN_TAG_DESCRIPTION_NO_FIELD},
{"name": "inTag", "description": IN_TAG_DESCRIPTION},
],
"Args": [
{"name": "aOffset", "description": "memory offset of the operation's left input"},
Expand All @@ -149,7 +149,7 @@ const INSTRUCTION_SET_RAW = [
"Category": "Compute - Comparators",
"Flags": [
{"name": "indirect", "description": INDIRECT_FLAG_DESCRIPTION},
{"name": "inTag", "description": IN_TAG_DESCRIPTION_NO_FIELD},
fcarreiro marked this conversation as resolved.
Show resolved Hide resolved
{"name": "inTag", "description": IN_TAG_DESCRIPTION},
],
"Args": [
{"name": "aOffset", "description": "memory offset of the operation's left input"},
Expand Down
Loading