AVM: match, pushints, and pushbytess opcodes #4645

algoidurovic · 2022-10-14T16:44:04Z

Summary

Adding the match opcode, which is an extension of the switch opcode, where the later is a dense switch and the former a sparse switch. The match opcode is also able to match on bytes values, where switch only supports ints. The pushints and pushbytess were added to reduce the size of encoding match cases/constants as stack arguments. The alternative is to push the match cases onto the stack individually, taking up more space to store the additional opcodes. It's possible that this approach is better in some cases if the constants are stored in constant blocks so HLL compilers will need to be aware of this.

Test Plan

Assembly and evaluation tests.

jannotti · 2022-10-14T20:07:43Z

data/transactions/logic/TEAL_opcodes.md

+- construct a list of match cases from B_1 to B_N and branch to the Ith label where B_I = A. Continue to the following instruction if no matches are found.
+- Availability: v8
+
+`match` consumes N+1 values from the stack. Let the top stack value be A. The following N values represent an ordered list of match cases/constants (B), where the first value (B[0]) is the deepest in the stack. The immediate arguments are an ordered list of N labels (L). `match` will branch to the L[I], where B[I] = A. If there are no matches then execution continues on to the next instruction.


We call it B_1 to B_N above, then B[0] in description.

jannotti · 2022-10-14T20:10:05Z

data/transactions/logic/assembler.go

 		l = binary.PutUvarint(scratch[:], cu)
 		ops.pending.Write(scratch[:l])
-		if !ops.known.deadcode {
+		if !ops.known.deadcode { // this conditional doesn't seem necessary


It allows intcblock in deadcode to exist without mucking up the assemblers belief about what is in the cblock.

jannotti · 2022-10-14T20:29:26Z

data/transactions/logic/eval_test.go

+int 0
+int 1
+int 0
+match zero one


Let's do these test with big weird numbers like 37 and 62 or some such, so it's obvious at a glance that we're not talking about indices into the labels.

jannotti · 2022-10-14T20:31:23Z

data/transactions/logic/eval.go

+	matchVal := cx.stack[top]
+	cx.stack = cx.stack[:top]
+
+	top = len(cx.stack) - n


Weird to call this top. It's the base of the match values, right?

Yes, I was thinking of top as representing the new top of the stack after the match cases are removed from the stack but I can see how that's imprecise/confusing.

data/transactions/logic/eval.go

codecov · 2022-10-17T17:23:06Z

Codecov Report

Merging #4645 (97b008e) into master (db7d9c3) will increase coverage by 0.04%.
The diff coverage is 84.88%.

@@            Coverage Diff             @@
##           master    #4645      +/-   ##
==========================================
+ Coverage   54.33%   54.37%   +0.04%     
==========================================
  Files         402      402              
  Lines       51828    51859      +31     
==========================================
+ Hits        28159    28198      +39     
+ Misses      21297    21292       -5     
+ Partials     2372     2369       -3

Impacted Files	Coverage Δ
data/transactions/logic/doc.go	`61.53% <ø> (ø)`
data/transactions/logic/opcodes.go	`84.56% <ø> (ø)`
data/transactions/logic/assembler.go	`86.37% <81.57%> (+0.97%)`	⬆️
data/transactions/logic/eval.go	`89.35% <87.23%> (-0.05%)`	⬇️
data/transactions/logic/frames.go	`100.00% <100.00%> (ø)`
cmd/algoh/blockWatcher.go	`77.77% <0.00%> (-3.18%)`	⬇️
network/wsPeer.go	`65.50% <0.00%> (-2.14%)`	⬇️
data/transactions/verify/txn.go	`76.19% <0.00%> (-0.96%)`	⬇️
ledger/tracker.go	`74.04% <0.00%> (-0.86%)`	⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jannotti · 2022-10-17T18:29:41Z

data/transactions/logic/TEAL_opcodes.md


+## pushbytess bytes ...
+
+- Opcode: 0x82 {varuint length} [({varuint value length} bytes), ...]


How about we change the first length to count?

For consistency, we would do this for the cblock opcodes and pushints too.

jannotti · 2022-10-17T18:30:56Z

data/transactions/logic/TEAL_opcodes.md

+
+- Opcode: 0x8e {uint8 branch count} [{int16 branch offset, big-endian}, ...]
+- Stack: ..., [N items] &rarr; ...
+- construct a list of match cases from B[0] to B[N] and branch to the Ith label where B[I] = A. Continue to the following instruction if no matches are found.


B[0] to B[N] implies N+1 targets.

I'd avoid talk of "construct a list"

The input stack doesn't seem right. It should have N items and a value, right?

You're right, seems I've been sloppy with the docs.

data/transactions/logic/doc.go

jannotti · 2022-10-17T18:42:07Z

data/transactions/logic/eval.go

+	finalLen := len(cx.stack) + len(intc)
+	if cap(cx.stack) < finalLen {
+		// Let's grow all at once, plus a little slack.
+		newStack := make([]stackValue, len(cx.stack), finalLen+4)
+		copy(newStack, cx.stack)
+		cx.stack = newStack
+	}


I think we're doing this in three places now. Maybe we should have a function, grow or ensure?

jannotti · 2022-10-17T18:44:44Z

data/transactions/logic/eval.go

+	// first we must verify that the matchList has the expected length and all types match the type of matchVal
+	if len(matchList) != n {
+		return fmt.Errorf("list of match constants has length %d when length %d was expected", len(matchList), n)
+	}


This is impossible, right? You've confirmed the stack is big enough, then sliced it up properly.

(Certainly the comment is out of date.)

You're right, the check and comment are both out of date.

data/transactions/logic/eval.go

jannotti · 2022-10-17T18:52:34Z

data/transactions/logic/opcodes.go

 	{0x8c, "frame_bury", opFrameBury, proto("a:"), fpVersion, immKinded(immInt8, "i").typed(typeFrameBury)},
 	{0x8d, "switch", opSwitch, proto("i:"), 8, detSwitch()},
-	// 0x8e will likely be a switch on pairs of values/targets, called `match`
+	{0x8e, "match", opMatch, proto(":", "[N items]", ""), 8, detSwitch().trust()},


Here's where you need something like, "[N items], A".

Actually, how about using:
"[T1 T2 T3 ... TN], B"
That way you have names for the targets (which I renamed T for target instead of B for branch to prevent confusion because we normally named the arguments A and B with A deeper in the stack).

That should make your documentation of the behavior easier.

oops, they are not targets, they are match values. So I'd be ok with "[A1 A2 A3 ... An], B"

data/transactions/logic/assembler_test.go

data/transactions/logic/eval.go

jannotti · 2022-10-18T17:05:33Z

data/transactions/logic/README.md

 | `bytec_2` | constant 2 from bytecblock |
 | `bytec_3` | constant 3 from bytecblock |
 | `pushbytes bytes` | immediate BYTES |
+| `pushbytess bytes ...` | push sequences of immediate bytes to stack |


I guess this should also say "(first byte array being deepest)"

PS, we try to consistently say "byte array" rather than "bytes".

jannotti · 2022-10-18T17:07:19Z

data/transactions/logic/TEAL_opcodes.md

 ## pushbytes bytes

- Opcode: 0x80 {varuint length} {bytes}
+- Opcode: 0x80 {varuint count} {bytes}


I don't think this one should be changed. It's the length of the byte array, not the count of how many byte arrays there are.

jannotti · 2022-10-18T17:08:37Z

data/transactions/logic/TEAL_opcodes.md

+- Stack: ..., [A1, A2, ..., AN], B &rarr; ...
+- given match cases from A[0] to A[N-1], branch to the Ith label where A[I] = B. Continue to the following instruction if no matches are found.


Still some disagreement on whether A is 0 based or 1 based. I vote for 1 based, only so the "Stack:" line can avoid using "N-1" as a "subscript".

jannotti · 2022-10-18T17:15:34Z

data/transactions/logic/eval.go

+			continue
+		}
+
+		if matchVal.argType() == StackBytes && string(matchVal.Bytes) == string(stackArg.Bytes) {


Suggested change

if matchVal.argType() == StackBytes && string(matchVal.Bytes) == string(stackArg.Bytes) {

if matchVal.argType() == StackBytes && bytes.Equal(matchVal.Bytes, stackArg.Bytes) {

jannotti · 2022-10-20T14:55:59Z

data/transactions/logic/TEAL_opcodes.md

+- push sequences of immediate byte arrays to stack (first byte array being deepest)
+- Availability: v8
+
+pushbytess args are not added to the bytecblock during assembly processes


Do you think we need these caveats? I'd rather remove them.

I don't mind removing it. I thought that it'd be useful to communicate in some way that these opcodes aren't necessarily more space efficient than a series of byte/int instructions. pushbytes/pushint have the same caveat in the docs.

jannotti · 2022-10-20T15:08:42Z

data/transactions/logic/eval.go

+	matchedIdx := n
+	for i, stackArg := range matchList {
+		if stackArg.argType() != matchVal.argType() {
+			continue


Do we not have any tests that mix bytes and ints?

Added more to the eval tests.

algoidurovic added 3 commits October 7, 2022 13:08

add match opcode along with assembler and eval unit tests

84e9756

initial implementation of match, pushints, and pushbytess

bc8f498

add docs for pushints and pushbytess

c5d52f8

algoidurovic requested a review from jannotti October 14, 2022 16:44

fix test error message

6943ac3

algoidurovic added the New Feature label Oct 14, 2022

error msg

bc323b0

jannotti reviewed Oct 14, 2022

View reviewed changes

touch up

f51ea3a

algoidurovic requested a review from jannotti October 17, 2022 17:26

jannotti reviewed Oct 17, 2022

View reviewed changes

respond to feedback

ebb5e21

jannotti reviewed Oct 17, 2022

View reviewed changes

data/transactions/logic/assembler_test.go Show resolved Hide resolved

data/transactions/logic/assembler_test.go Show resolved Hide resolved

data/transactions/logic/eval.go Show resolved Hide resolved

update docs and improve tests

ffa28dd

algoidurovic requested a review from jannotti October 18, 2022 16:50

jannotti reviewed Oct 18, 2022

View reviewed changes

algoidurovic added 2 commits October 18, 2022 13:33

tweak

c65bec4

typo

df29895

algoidurovic requested a review from jannotti October 18, 2022 17:38

michaeldiamant mentioned this pull request Oct 19, 2022

Add Match Opcode #4626

Closed

jannotti reviewed Oct 20, 2022

View reviewed changes

add mixed type match tests

97b008e

algoidurovic requested a review from jannotti October 21, 2022 16:55

jannotti approved these changes Oct 26, 2022

View reviewed changes

jannotti merged commit ab87a8a into algorand:master Oct 26, 2022

Algo-devops-service mentioned this pull request Nov 1, 2022

go-algorand 3.12.0-beta Release PR #4730

Merged

Algo-devops-service mentioned this pull request Nov 10, 2022

go-algorand 3.12.0-stable Release PR #4783

Closed

Algo-devops-service mentioned this pull request Nov 28, 2022

go-algorand 3.12.2-stable Release PR #4837

Merged


		## pushbytess bytes ...

		- Opcode: 0x82 {varuint length} [({varuint value length} bytes), ...]

		- Stack: ..., [A1, A2, ..., AN], B → ...
		- given match cases from A[0] to A[N-1], branch to the Ith label where A[I] = B. Continue to the following instruction if no matches are found.

	if matchVal.argType() == StackBytes && string(matchVal.Bytes) == string(stackArg.Bytes) {
	if matchVal.argType() == StackBytes && bytes.Equal(matchVal.Bytes, stackArg.Bytes) {

AVM: match, pushints, and pushbytess opcodes #4645

AVM: match, pushints, and pushbytess opcodes #4645

Uh oh!

Conversation

algoidurovic commented Oct 14, 2022

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 17, 2022 •

edited

Loading

jannotti Oct 17, 2022 •

edited

Loading

jannotti Oct 18, 2022 •

edited

Loading