Skip to content

Conversation

@marchioa
Copy link
Contributor

@marchioa marchioa commented Oct 21, 2025

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

PR Merge Checklist

  1. The PR is rebased on the latest devel commit and pointing to devel.
  2. Your PR reviewed and approved.
  3. All checks are passing.
  4. The CHANGELOG.md file has been updated.
  5. If the docker was modified, change back its link after review.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

📝 Walkthrough

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed bias hoisting behavior in generic GEMM operations when no bias tensor is present.
  • Changes

    • Updated default bias hoisting configuration for generic GEMM operations.

Walkthrough

GEMM 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

Cohort / File(s) Summary
GEMM Parser Refactor
Deeploy/Targets/Generic/Parsers.py
- GEMMParser.__init__ now calls super().__init__(noBiasHoisting) and no longer assigns self.noBiasHoisting directly.
- Removed creation/injection of synthetic C (bias) tensors and mock bias matrices in parseNodeCtxt when bias hoisting was disabled.
- parseNode now explicitly creates a bias C tensor when there are exactly 2 inputs and bias hoisting is disabled.
- When 3 inputs are present, existing C handling remains; the prior fallback that synthesized a mock bias for the no-hoist path was removed.
- GenericGEMMParser.__init__ default noBiasHoisting changed from True to False.
Changelog
CHANGELOG.md
- Added entry documenting a fix for bias hoisting in generic GEMM with no bias.

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)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Potential review focus:

  • Consistency of C tensor shape and C_batched flags across parseNode and parseNodeCtxt.
  • Contract between GEMMParser and base parser regarding noBiasHoisting.
  • Call sites or tests that expected automatic synthetic C insertion when hoisting was disabled.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix bug in Generic GEMM with no bias" directly and accurately reflects the main changes in the changeset. The raw summary confirms that the changes center on fixing bias hoisting issues in generic GEMM when no bias is present, with modifications to the initialization flow and removal of synthetic bias tensor creation paths. The title is concise, specific, and clearly conveys the primary change from the developer's perspective without being vague or misleading.
Description Check ✅ Passed The pull request description provided by the author clearly relates to the changeset and describes the specific changes made. The description accurately identifies the issue (GEMM not correctly hoisting the bias tensor in the generic platform), explains the solution (moving bias hoisting from MatMulParser.parseNodeCtxt to GEMMParser.parseNode), and mentions the adjustment to GenericGEMMParser's noBiasHoisting default from True to False. These elements directly correspond to the changes documented in the raw summary of the Parsers.py file and the CHANGELOG.md update. The description is neither vague nor off-topic—it provides meaningful context about the purpose and scope of the changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 15c4a23 and 0641fc9.

📒 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 noBiasHoisting to MatMulParser.__init__, which stores it as self.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 test testFloatGEMMnobias mentioned 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 noBiasHoisting default from True to False is 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:

  1. Confirm whether the test mentioned in the PR description (testFloatGEMMnobias) was intended to be added as part of this PR
  2. Ensure existing GEMM test suite passes with the new default behavior
  3. Document the breaking change or update code to explicitly pass noBiasHoisting=True to maintain backward compatibility

@Xeratec Xeratec added the Bug Something isn't working label Oct 23, 2025
@Xeratec Xeratec added this to Deeploy Oct 23, 2025
@Xeratec Xeratec moved this to Need Reviewer in Deeploy Oct 23, 2025
@Xeratec Xeratec added this to the Release 0.2.1 milestone Oct 23, 2025
@Xeratec Xeratec moved this from Need Reviewer to In review in Deeploy Oct 28, 2025
Copy link
Collaborator

@lukamac lukamac left a 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:

  1. Add your change to the changelog
  2. 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 -i and just pick your commits.

@lukamac lukamac self-requested a review October 30, 2025 16:31
Copy link
Collaborator

@lukamac lukamac left a comment

Choose a reason for hiding this comment

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

lgtm

@lukamac lukamac merged commit 23e9f02 into pulp-platform:devel Oct 31, 2025
134 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Deeploy Oct 31, 2025
RiccardoGandolfi pushed a commit to FondazioneChipsIT/Deeploy that referenced this pull request Nov 3, 2025
 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]>
marchioa added a commit to FondazioneChipsIT/Deeploy that referenced this pull request Nov 3, 2025
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]>
RiccardoGandolfi added a commit to FondazioneChipsIT/Deeploy that referenced this pull request Nov 3, 2025
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants