-
Couldn't load subscription status.
- Fork 23
fix bug in Generic GEMM with no bias #126
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
base: devel
Are you sure you want to change the base?
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request refactors GEMM parser initialization to delegate bias-hoisting handling to the base class, removes synthetic C tensor creation when bias hoisting is disabled, and changes the default bias-hoisting behavior in GenericGEMMParser from enabled to disabled. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Deeploy/Targets/Generic/Parsers.py(3 hunks)
🔇 Additional comments (2)
Deeploy/Targets/Generic/Parsers.py (2)
1738-1739: LGTM: Initialization now delegates to base class.The initialization correctly passes
noBiasHoistingtoMatMulParser.__init__, which stores it asself.noBiasHoisting. This is cleaner than the previous pattern.
2269-2269: I need to search the codebase more thoroughly to understand the impact of this change.Based on the evidence gathered, I can now generate the rewritten review comment. The key finding is that the testtestFloatGEMMnobiasmentioned in the PR description does NOT exist in the codebase, and the default parameter change is a BREAKING CHANGE affecting two platforms.
The PR claims to add
testFloatGEMMnobias, but this test does not exist in the codebase. Verify test coverage before merging.The change to
noBiasHoistingdefault fromTruetoFalseis a breaking behavioral change affecting GEMM operations on two platforms:
Deeploy/Targets/Generic/Platform.py:41(Generic platform)Deeploy/Targets/MemPool/Platform.py:74(MemPool platform)Both instantiations rely on the default parameter, so they will now enable bias hoisting by default instead of disabling it.
Required actions:
- Confirm whether the test mentioned in the PR description (
testFloatGEMMnobias) was intended to be added as part of this PR- Ensure existing GEMM test suite passes with the new default behavior
- Document the breaking change or update code to explicitly pass
noBiasHoisting=Trueto maintain backward compatibility
In generic platform, GEMM was not correctly hoisting the bias tensor when required.
To solve the issue, bias hoisting has been moved from
MatMulParser.parseNodeCtxttoGEMMParser.parseNode.Moreover, the default value of
noBiasHoistingflag inGenericGEMMParserhas been changed from True to False to be compliant with the template.Added
Changed
MatMulParser,GEMMParser, andGenericGEMMParser)Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.