Skip to content

Conversation

@gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Apr 10, 2025

Description

Fields that are smaller than 31bits now use uint32 by default as word representation. Mostly to address this: Consensys/gnark#1463 (comment) ;

if this causes a breaking change (i.e. code relying on gnark-crypto to generate a tiny field and expecting uint64) then we should refactor a bit NewFieldConfig to take options and take as an option argument a way to force the word size.

@gbotrel gbotrel requested review from Copilot and ivokub April 10, 2025 16:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • field/asm/.gitignore: Language not supported
Comments suppressed due to low confidence (3)

field/generator/internal/templates/element/tests.go:1587

  • [nitpick] Ensure that the new type 'testPair{{.ElementName}}' is defined and used consistently throughout the tests for clarity.
func(a testPair{{.ElementName}}) bool {

field/generator/asm/arm64/build.go:60

  • Verify that 'GenerateF31ASM' correctly handles scenarios when nbBits is less than 31, as the condition now triggers for all nbBits ≤ 31.
if nbBits <= 31 {

field/generator/asm/amd64/build.go:249

  • Ensure that 'GenerateF31ASM' functions properly for cases where nbBits < 31, given that the updated condition now covers these cases.
if nbBits <= 31 {

@ivokub
Copy link
Collaborator

ivokub commented Apr 11, 2025

Hmm, it still breaks gnark for now. There is some handling of the options/configuration such that we still compile asm instructions for vector and element, but if we write the output to gnark, then the include directive breaks.

I tried a bit to make it work and now it seems we don't generate asm instructions at all:

-       g.Go(generate("element_amd64.s", []string{element.IncludeASM}, only(F.GenerateOpsAMD64), withBuildTag("!purego"), withData(amd64d)))
-       g.Go(generate("element_arm64.s", []string{element.IncludeASM}, only(F.GenerateOpsARM64), withBuildTag("!purego"), withData(arm64d)))
+       g.Go(generate("element_amd64.s", []string{element.IncludeASM}, only(F.GenerateOpsAMD64 && hashAMD64 != ""), withBuildTag("!purego"), withData(amd64d)))
+       g.Go(generate("element_arm64.s", []string{element.IncludeASM}, only(F.GenerateOpsARM64 && hashArm64 != ""), withBuildTag("!purego"), withData(arm64d)))
 
-       g.Go(generate("element_amd64.go", []string{element.OpsAMD64, element.MulDoc}, only(F.GenerateOpsAMD64 && !F.F31), withBuildTag("!purego")))
-       g.Go(generate("element_arm64.go", []string{element.OpsARM64, element.MulNoCarry, element.Reduce}, only(F.GenerateOpsARM64 && !F.F31), withBuildTag("!purego")))
+       g.Go(generate("element_amd64.go", []string{element.OpsAMD64, element.MulDoc}, only(F.GenerateOpsAMD64 && !F.F31 && hashAMD64 != ""), withBuildTag("!purego")))
+       g.Go(generate("element_arm64.go", []string{element.OpsARM64, element.MulNoCarry, element.Reduce}, only(F.GenerateOpsARM64 && !F.F31 && hashArm64 != ""), withBuildTag("!purego")))
 
        g.Go(generate("element_purego.go", []string{element.OpsNoAsm, element.MulCIOS, element.MulNoCarry, element.Reduce, element.MulDoc}, withBuildTag(pureGoBuildTag)))
 
-       g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64}, only(F.GenerateVectorOpsAMD64 && !F.F31), withBuildTag("!purego")))
-       g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64F31}, only(F.GenerateVectorOpsAMD64 && F.F31), withBuildTag("!purego")))
-       g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64}, only(F.GenerateVectorOpsARM64 && !F.F31), withBuildTag("!purego")))
-       g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64F31}, only(F.GenerateVectorOpsARM64 && F.F31), withBuildTag("!purego")))
+       g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64}, only(F.GenerateVectorOpsAMD64 && !F.F31 && hashAMD64 != ""), withBuildTag("!purego")))
+       g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64F31}, only(F.GenerateVectorOpsAMD64 && F.F31 && hashAMD64 != ""), withBuildTag("!purego")))
+       g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64}, only(F.GenerateVectorOpsARM64 && !F.F31 && hashArm64 != ""), withBuildTag("!purego")))
+       g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64F31}, only(F.GenerateVectorOpsARM64 && F.F31 && hashArm64 != ""), withBuildTag("!purego")))

(when I use generator.GenerateFF(conf, d.RootPath, generator.WithASM(nil)) in gnark to generate tinyfield).

But now the problem is that the generic implementations always have purego build tag and we don't have a fallback.

@ivokub
Copy link
Collaborator

ivokub commented Apr 11, 2025

Suggested edit:

diff --git a/field/generator/generator_field.go b/field/generator/generator_field.go
index 9e2cd5969..bf0acb792 100644
--- a/field/generator/generator_field.go
+++ b/field/generator/generator_field.go
@@ -143,6 +143,7 @@ func generateField(F *config.Field, outputDir, asmDirIncludePath, hashArm64, has
 
 	if F.F31 {
 		pureGoBuildTag = "" // always generate pure go for F31
+		pureGoVectorBuildTag = ""
 	}
 
 	var g errgroup.Group
@@ -154,18 +155,18 @@ func generateField(F *config.Field, outputDir, asmDirIncludePath, hashArm64, has
 	g.Go(generate("element_test.go", testFiles))
 	g.Go(generate("vector_test.go", []string{element.TestVector}))
 
-	g.Go(generate("element_amd64.s", []string{element.IncludeASM}, only(F.GenerateOpsAMD64), withBuildTag("!purego"), withData(amd64d)))
-	g.Go(generate("element_arm64.s", []string{element.IncludeASM}, only(F.GenerateOpsARM64), withBuildTag("!purego"), withData(arm64d)))
+	g.Go(generate("element_amd64.s", []string{element.IncludeASM}, only(F.GenerateOpsAMD64 && hashAMD64 != ""), withBuildTag("!purego"), withData(amd64d)))
+	g.Go(generate("element_arm64.s", []string{element.IncludeASM}, only(F.GenerateOpsARM64 && hashArm64 != ""), withBuildTag("!purego"), withData(arm64d)))
 
-	g.Go(generate("element_amd64.go", []string{element.OpsAMD64, element.MulDoc}, only(F.GenerateOpsAMD64 && !F.F31), withBuildTag("!purego")))
-	g.Go(generate("element_arm64.go", []string{element.OpsARM64, element.MulNoCarry, element.Reduce}, only(F.GenerateOpsARM64 && !F.F31), withBuildTag("!purego")))
+	g.Go(generate("element_amd64.go", []string{element.OpsAMD64, element.MulDoc}, only(F.GenerateOpsAMD64 && !F.F31 && hashAMD64 != ""), withBuildTag("!purego")))
+	g.Go(generate("element_arm64.go", []string{element.OpsARM64, element.MulNoCarry, element.Reduce}, only(F.GenerateOpsARM64 && !F.F31 && hashArm64 != ""), withBuildTag("!purego")))
 
 	g.Go(generate("element_purego.go", []string{element.OpsNoAsm, element.MulCIOS, element.MulNoCarry, element.Reduce, element.MulDoc}, withBuildTag(pureGoBuildTag)))
 
-	g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64}, only(F.GenerateVectorOpsAMD64 && !F.F31), withBuildTag("!purego")))
-	g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64F31}, only(F.GenerateVectorOpsAMD64 && F.F31), withBuildTag("!purego")))
-	g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64}, only(F.GenerateVectorOpsARM64 && !F.F31), withBuildTag("!purego")))
-	g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64F31}, only(F.GenerateVectorOpsARM64 && F.F31), withBuildTag("!purego")))
+	g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64}, only(F.GenerateVectorOpsAMD64 && !F.F31 && hashAMD64 != ""), withBuildTag("!purego")))
+	g.Go(generate("vector_amd64.go", []string{element.VectorOpsAmd64F31}, only(F.GenerateVectorOpsAMD64 && F.F31 && hashAMD64 != ""), withBuildTag("!purego")))
+	g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64}, only(F.GenerateVectorOpsARM64 && !F.F31 && hashArm64 != ""), withBuildTag("!purego")))
+	g.Go(generate("vector_arm64.go", []string{element.VectorOpsArm64F31}, only(F.GenerateVectorOpsARM64 && F.F31 && hashArm64 != ""), withBuildTag("!purego")))
 
 	g.Go(generate("vector_purego.go", []string{element.VectorOpsPureGo}, withBuildTag(pureGoVectorBuildTag)))
 

@ivokub
Copy link
Collaborator

ivokub commented Apr 11, 2025

The above edit seems to fix it, I overlooked it.

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 to me now. And seems that also works with gnark side Consensys/gnark#1463.

Have a look yourself also - I modified a bit the field generation to avoid generating assembly when the assembly configuration is not given. The issue is that the Field configuration has the flags for generating assembly even if we don't give the configuration (output path etc). I added toggles with hashAMD64 and hashArm64. Another option is to modify config.Field, but as we pass it as reference, then it would mutate it which imo isn't perfect.

In any case - I still try to wrap up the requested changes in gnark side, lets try to merge these two at the same time.

@ivokub ivokub merged commit 685fcab into master Apr 13, 2025
5 checks passed
@ivokub ivokub deleted the feat/f32 branch April 13, 2025 23:02
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