Fix x64 tests with GFNI enabled, remove Canonical type#887
Closed
Fix x64 tests with GFNI enabled, remove Canonical type#887
Conversation
Contributor
Author
How to use the Graphite Merge QueueAdd the label merge-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
jimpo
approved these changes
Sep 1, 2025
jadnohra
approved these changes
Sep 1, 2025
GraDKh
changed the base branch from
dgordon/test_against_native_cpu_in_ci
to
graphite-base/887
GraDKh
force-pushed
the
dgordon/fix_x64_tests_with_gfni
branch
from
996fe65 to
380aa9b
Compare
GraDKh
force-pushed
the
graphite-base/887
branch
from
71ae714 to
493e2ff
Compare
Merge activity
|
graphite-app bot
pushed a commit
that referenced
this pull request
Sep 2, 2025
### TL;DR Remove the `Canonical` associated type from the `TowerField` trait and related code. This fixes the unit tests that were not passing with GFNI enabled because `is_canonical` was returning a wrong value ### What changed? - Removed the `Canonical` associated type from the `TowerField` trait - Removed implementations of this associated type from all implementors of `TowerField` - Removed the `is_canonical_tower` function and its usages - Removed the tower-to-AES and AES-to-tower mapping constants that were used for conversions - Removed the `linear_transform` function that was no longer needed - Simplified the `invert_or_zero` implementation to only handle AES tower fields without conversion ### How to test? - Run the existing test suite to ensure all functionality still works correctly - Verify that code using binary fields still compiles and functions as expected - Check that GFNI-based field operations still work correctly ### Why make this change? This change simplifies the binary field implementation by removing the concept of "canonical" tower fields. The code previously supported conversions between different representations of tower fields, but this added complexity wasn't necessary. By removing this abstraction, the code becomes more straightforward and easier to maintain while preserving all required functionality.
lockedloop
pushed a commit
that referenced
this pull request
Sep 8, 2025
### TL;DR Remove the `Canonical` associated type from the `TowerField` trait and related code. This fixes the unit tests that were not passing with GFNI enabled because `is_canonical` was returning a wrong value ### What changed? - Removed the `Canonical` associated type from the `TowerField` trait - Removed implementations of this associated type from all implementors of `TowerField` - Removed the `is_canonical_tower` function and its usages - Removed the tower-to-AES and AES-to-tower mapping constants that were used for conversions - Removed the `linear_transform` function that was no longer needed - Simplified the `invert_or_zero` implementation to only handle AES tower fields without conversion ### How to test? - Run the existing test suite to ensure all functionality still works correctly - Verify that code using binary fields still compiles and functions as expected - Check that GFNI-based field operations still work correctly ### Why make this change? This change simplifies the binary field implementation by removing the concept of "canonical" tower fields. The code previously supported conversions between different representations of tower fields, but this added complexity wasn't necessary. By removing this abstraction, the code becomes more straightforward and easier to maintain while preserving all required functionality.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Remove the
Canonicalassociated type from theTowerFieldtrait and related code. This fixes the unit tests that were not passing with GFNI enabled becauseis_canonicalwas returning a wrong valueWhat changed?
Canonicalassociated type from theTowerFieldtraitTowerFieldis_canonical_towerfunction and its usageslinear_transformfunction that was no longer neededinvert_or_zeroimplementation to only handle AES tower fields without conversionHow to test?
Why make this change?
This change simplifies the binary field implementation by removing the concept of "canonical" tower fields. The code previously supported conversions between different representations of tower fields, but this added complexity wasn't necessary. By removing this abstraction, the code becomes more straightforward and easier to maintain while preserving all required functionality.