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

Legalize neg: -x becomes 0 - x #2128

Merged
merged 2 commits into from
Mar 19, 2021
Merged

Legalize neg: -x becomes 0 - x #2128

merged 2 commits into from
Mar 19, 2021

Conversation

jackkoenig
Copy link
Contributor

This fixes an error with negating a negative SInt literal and a
[debatable] lint warning in Verilator when negating any value.

This behavior matches that of Chisel (which directly emits the 0 - x
already).

Fixes #2026 although it introduces a new lint warning for that case
Fixes #2031

Follow on work to do constant propagation on subtraction (which we somehow are still not doing) which will fix the introduced lint warning (but a lint warning is better than an error!)

Contributor Checklist

  • 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
  • backend code generation

API Impact

Depending on Legalize means a transform will no longer see Neg prim ops.

Backend Code Generation Impact

This changes the emission of neg from:

-$signed(x);

to

8'h0 - $signed(x);

As described in #2031, Verilator has a lint warning on this (warnings default to error). I don't agree with the lint warning, but since Chisel doesn't emit neg anyway (instead emitting 0 - x like this change), I think matching the behavior of Chisel is fine.

We should consider seeing if Verilator might change that warning though.

Desired Merge Strategy

  • Squash

Release Notes

Canonicalize neg(x) to 0 - x. Fix bug where negated negative literal would result in invalid Verilog.

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?

This fixes an error with negating a negative SInt literal and a
[debatable] lint warning in Verilator when negating any value.

This behavior matches that of Chisel (which directly emits the 0 - x
already).
@jackkoenig jackkoenig added this to the 1.4.x milestone Mar 19, 2021
@jackkoenig jackkoenig merged commit 49b8232 into master Mar 19, 2021
mergify bot pushed a commit that referenced this pull request Mar 19, 2021
This fixes an error with negating a negative SInt literal and a
[debatable] lint warning in Verilator when negating any value.

This behavior matches that of Chisel (which directly emits the 0 - x
already).

(cherry picked from commit 49b8232)
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Mar 19, 2021
mergify bot added a commit that referenced this pull request Mar 19, 2021
This fixes an error with negating a negative SInt literal and a
[debatable] lint warning in Verilator when negating any value.

This behavior matches that of Chisel (which directly emits the 0 - x
already).

(cherry picked from commit 49b8232)

Co-authored-by: Jack Koenig <[email protected]>
@jackkoenig jackkoenig deleted the legalize-neg branch March 26, 2021 23:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backported This PR has been backported to marked stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(neg) lint warning double negative sign
2 participants