Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Fix width of signed addition when input to mux #2440

Merged
merged 2 commits into from
Dec 18, 2021
Merged

Fix width of signed addition when input to mux #2440

merged 2 commits into from
Dec 18, 2021

Conversation

jackkoenig
Copy link
Contributor

Extremely focused fix for #2439. I'm not sure if this is the best fix but I think the test is pretty good at least so wanted to put this out there.

Fixes #2439

h/t to @youngar for finding an OG bug.

I'm wondering if it would be better to just remove the casts in the VerilogEmitter, do we ever actually need to cast the inputs to a mux?

firrtl/src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala

Line 182 in 4347f57

emitCol(Seq(e.cond, " ? ", cast(e.tval), " : ", cast(e.fval)), top + 1, tabs, colNum)

That's just a lot scarier of a change 😨

I'm backporting this as far as it will go...

Contributor Checklist

  • [NA] Did you add Scaladoc to every public function/method?
  • [NA] Did you update the FIRRTL spec to include every new feature/behavior?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • bug fix

API Impact

No impact

Backend Code Generation Impact

Minimal, will split out signed addition into another Statement if it is a direct input to a mux

Desired Merge Strategy

  • Squash

Release Notes

Fix width of signed addition when it is an input to a mux.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@jackkoenig jackkoenig added this to the 1.2.x milestone Dec 9, 2021
seldridge
seldridge reviewed Dec 9, 2021
View reviewed changes
src/main/scala/firrtl/passes/SplitExpressions.scala Outdated
Comment on lines 30 to 36
private def isSignedAdd(e: Expression): Boolean = e match {
case DoPrim(PrimOps.Add, _, _, _: SIntType) => true
case _ => false
}
Copy link
Member

Choose a reason for hiding this comment

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

This may be prefetching, too far, but maybe we want to do this for any arithmetic op of SIntType? E.g., I can produce a similar error with subtraction:

https://www.edaplayground.com/x/WAHv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely not prefetching too far. If you can produce the same error with other ops then we should fix those as well. I'm leaning toward the VerilogEmitter change instead since it should fix the whole class of issues but it makes me a little nervous.

@seldridge seldridge mentioned this pull request Dec 17, 2021
Closed
5 tasks
@jackkoenig @seldridge
Fix width of signed arithmetic when input to mux
2d119a2 
Fix bugs related to arithmetic ops inlined into a mux leg.  Add formal
equivalence checks to lock in this behavior.

Signed-off-by: Schuyler Eldridge <[email protected]>
@jackkoenig jackkoenig added the Skip Formal CI Skip the Formal Equivalence CI Checks label Dec 18, 2021
@jackkoenig
Copy link
Contributor Author

Skipping formal CI because it should fail

@jackkoenig
Copy link
Contributor Author

I added the label a bit late. Formal equiv for Ops failed (as it should because we're fixing it), now rerunning.

@seldridge seldridge merged commit c00a4eb into master Dec 18, 2021
@seldridge seldridge deleted the fix-2439 branch December 18, 2021 06:40
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Dec 18, 2021
@seldridge seldridge added Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. and removed Backported This PR has been backported to marked stable branch labels Dec 18, 2021
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Dec 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@seldridge seldridge seldridge approved these changes

Assignees
No one assigned
Labels
Backported This PR has been backported to marked stable branch Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. Skip Formal CI Skip the Formal Equivalence CI Checks
Projects
None yet
Milestone
1.2.x
Development

Successfully merging this pull request may close these issues.

Truncating the result of signed addition
2 participants
@jackkoenig @seldridge