-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix bug in Generic GEMM with no bias #126
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughGEMM parser initialization now delegates bias-hoisting to the base class. Code that synthesized fake C/bias tensors when bias hoisting was disabled was removed. parseNode gained explicit handling for the 2-input case when hoisting is disabled. GenericGEMMParser default noBiasHoisting changed to False. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant GEMMParser
participant BaseParser
Note over GEMMParser,BaseParser: Initialization
Caller->>GEMMParser: instantiate(noBiasHoisting)
GEMMParser->>BaseParser: super().__init__(noBiasHoisting)
Note right of BaseParser: Base handles bias-hoisting flag
Note over Caller,GEMMParser: parseNode (high-level)
Caller->>GEMMParser: parseNode(node)
alt node.inputs == 3
GEMMParser->>GEMMParser: use provided C (bias) tensor
else node.inputs == 2
alt noBiasHoisting == False
GEMMParser->>BaseParser: rely on base hoisting behavior (no synthetic C)
else noBiasHoisting == True
GEMMParser->>GEMMParser: create explicit C bias tensor for 2-input node
end
end
GEMMParser-->>Caller: parsed representation (with/without C)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential review focus:
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
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.
amazing work, beautiful!
Before it gets my stamp of approval, can you please make sure to:
- Add your change to the changelog
- Make sure your branch is rebased on devel, i.e., that your commits sit directly on top of the devel branch. I pulled locally your branch and you have been doing some merging. Try the command
git rebase -iand just pick your commits.
7185e32 to
df3b648
Compare
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.
lgtm
In generic platform, GEMM was not correctly hoisting the bias tensor when required. To solve the issue, bias hoisting has been moved from `MatMulParser.parseNodeCtxt` to `GEMMParser.parseNode`. Moreover, the default value of `noBiasHoisting` flag in `GenericGEMMParser` has been changed from True to False to be compliant with the template. ## Added - testFloatGEMMnobias ## Changed - Generic\Parser.py file (`MatMulParser`, `GEMMParser`, and `GenericGEMMParser`) ## Fixed - fix bias hoisting in GEMM with no bias Co-authored-by: Alex Marchioni <[email protected]>
In generic platform, GEMM was not correctly hoisting the bias tensor when required. To solve the issue, bias hoisting has been moved from `MatMulParser.parseNodeCtxt` to `GEMMParser.parseNode`. Moreover, the default value of `noBiasHoisting` flag in `GenericGEMMParser` has been changed from True to False to be compliant with the template. ## Added - testFloatGEMMnobias ## Changed - Generic\Parser.py file (`MatMulParser`, `GEMMParser`, and `GenericGEMMParser`) ## Fixed - fix bias hoisting in GEMM with no bias Co-authored-by: Alex Marchioni <[email protected]> Co-authored-by: Alex Marchioni <[email protected]>
…ws (#1) * CMAKE FIX: include pulp-open config file * Adding pulp open target for gvsoc emulation + Testrunner_tiled_PULPOpen.py * First commit for working iDMA after refactoring * Further changes for iDMA integration * Fix for event unit error when using iDMA + updated testrunners for pulp_open * First changes for pulp-open rtl simulation inside Deeploy * Updated CMake macro for pulp-open simulation on modelsim * fix bug in Generic GEMM with no bias (pulp-platform#126) In generic platform, GEMM was not correctly hoisting the bias tensor when required. To solve the issue, bias hoisting has been moved from `MatMulParser.parseNodeCtxt` to `GEMMParser.parseNode`. Moreover, the default value of `noBiasHoisting` flag in `GenericGEMMParser` has been changed from True to False to be compliant with the template. ## Added - testFloatGEMMnobias ## Changed - Generic\Parser.py file (`MatMulParser`, `GEMMParser`, and `GenericGEMMParser`) ## Fixed - fix bias hoisting in GEMM with no bias Co-authored-by: Alex Marchioni <[email protected]> * Added new platform: PULP OPEN + MCHAN * Updated changelog --------- Signed-off-by: RiccardoGandolfi <[email protected]> Co-authored-by: RiccardoGandolfi <[email protected]> Co-authored-by: RiccardoGandolfi <[email protected]> Co-authored-by: Alex Marchioni <[email protected]> Co-authored-by: Alex Marchioni <[email protected]>
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.