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

feat: adds api.MAC(..) #427

Merged
merged 18 commits into from Jan 12, 2023
Merged

feat: adds api.MAC(..) #427

merged 18 commits into from Jan 12, 2023

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Dec 22, 2022

	// MAC sets and return a = a + (b*c)
	// ! may mutate a without allocating a new result
	// ! always use MAC(...) result for correctness
	MAC(a, b, c Variable) Variable

Fixes #416 . Impact in std/math/emulated for rsh is significant (~40% less memallocs).

	for i := 0; i < len(bits); i++ {
		Σbi = api.MAC(Σbi, bits[i], c)
		ΣbiRShift = api.MAC(ΣbiRShift, bits[i], cRShift)

		c.Lsh(c, 1)
		cRShift.Lsh(cRShift, 1)
		api.AssertIsBoolean(bits[i])
	}

Also, this new api would result in less constraint in a PlonKish arithmetization, since it will create one constraint instead of 2.

@gbotrel gbotrel added consolidate strengthen an existing feature perf labels Dec 22, 2022
@gbotrel gbotrel added this to the v0.9.0 milestone Dec 22, 2022
@gbotrel gbotrel linked an issue Dec 22, 2022 that may be closed by this pull request
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Looks good. I think it is a clean and required change.

frontend/api.go Outdated Show resolved Hide resolved
std/math/emulated/wrapped_api.go Show resolved Hide resolved
test/engine.go Outdated Show resolved Hide resolved
func (builder *scs) MAC(a, b, c frontend.Variable) frontend.Variable {
// TODO can we do better here to limit allocations?
// technically we could do that in one PlonK constraint (against 2 for separate Add & Mul)
return builder.Add(a, builder.Mul(b, c))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it can be done. And this is also helpful for the case if we want to have a transpiler from some other circuit description to gnark.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Am going to wait on @ThomasPiellard new backend on that one; plonk frontend is in a need of a refactor to convert input to term, and add custom constraints etc... keeping the TODO around for now.

frontend/cs/r1cs/api.go Show resolved Hide resolved
frontend/cs/r1cs/api_assertions.go Show resolved Hide resolved
frontend/cs/r1cs/builder.go Outdated Show resolved Hide resolved
@ivokub
Copy link
Collaborator

ivokub commented Jan 9, 2023

Suggested edit:

diff --git a/std/math/emulated/wrapped_api.go b/std/math/emulated/wrapped_api.go
index aa433d24..6d40f4ee 100644
--- a/std/math/emulated/wrapped_api.go
+++ b/std/math/emulated/wrapped_api.go
@@ -77,8 +77,10 @@ func (w *FieldAPI[T]) Add(i1 frontend.Variable, i2 frontend.Variable, in ...fron
 }
 
 func (w *FieldAPI[T]) MulAcc(a, b, c frontend.Variable) frontend.Variable {
-	// TODO can we do better here to limit allocations?
-	return w.Add(a, w.Mul(b, c))
+	els := w.varsToElements(a, b, c)
+	mul := w.f.Mul(els[1], els[2])
+	res := w.f.Add(els[0], mul)
+	return res
 }
 
 func (w *FieldAPI[T]) Neg(i1 frontend.Variable) frontend.Variable {

@ivokub
Copy link
Collaborator

ivokub commented Jan 9, 2023

Suggested edit:

diff --git a/std/math/emulated/wrapped_api.go b/std/math/emulated/wrapped_api.go
index aa433d24..6d40f4ee 100644
--- a/std/math/emulated/wrapped_api.go
+++ b/std/math/emulated/wrapped_api.go
@@ -77,8 +77,10 @@ func (w *FieldAPI[T]) Add(i1 frontend.Variable, i2 frontend.Variable, in ...fron
 }
 
 func (w *FieldAPI[T]) MulAcc(a, b, c frontend.Variable) frontend.Variable {
-	// TODO can we do better here to limit allocations?
-	return w.Add(a, w.Mul(b, c))
+	els := w.varsToElements(a, b, c)
+	mul := w.f.Mul(els[1], els[2])
+	res := w.f.Add(els[0], mul)
+	return res
 }
 
 func (w *FieldAPI[T]) Neg(i1 frontend.Variable) frontend.Variable {

Tried to see if could reduce allocating new Element, but seems to be larger overhaul to have methods on Field type which also take the destination. I think it makes sense to do it separate PR.

@gbotrel gbotrel merged commit 5f837b0 into develop Jan 12, 2023
@gbotrel gbotrel deleted the feat/apiMAC branch January 12, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consolidate strengthen an existing feature perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add api.AddInPlace() and api.MulInPlace
2 participants