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

Add --dont-fold option to disable folding specific prim ops #2040

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

seldridge
Copy link
Member

@seldridge seldridge commented Jan 16, 2021

This adds a --dont-fold options (backed by a DisableFold annotation)
that lets a user specify primitive operations which should never be
folded. This feature lets a user disable certain folds which may be
allowable in FIRRTL (or by any sane synthesis tool), but due to inane
Verilog language design causes formal equivalence tools to fail due to
the fold.

Add a test that a user can disable a / a -> 1 with a
DisableFold(PrimOps.Div) annotation.

Fixes #2029.

Example Usage

The following circuit:

circuit Foo :
  module Foo :
    input a: UInt<8>
    output b: UInt<8>

    b <= div(a, a)

When normally compiled will have div(a, a) legally optimized to 1 by taking advantage of the fact that division by zero is undefined in the FIRRTL spec (and arguably is implicitly undefined for synthesis in Verilog...):

firrtl -i Foo.fir
module Foo(
  input  [7:0] a,
  output [7:0] b
);
  assign b = 8'h1;
endmodule

If a user disables folding of division:

firrtl -i Foo.fir --dont-fold div

The user gets an a / a in the output:

module Foo(
  input  [7:0] a,
  output [7:0] b
);
  assign b = a / a;
endmodule

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • [n/a] 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

  • new feature/API

API Impact

This is a pure command line / annotation API addition.

Backend Code Generation Impact

None by default. A user can elect to change the backend code generation with the new --dont-fold option.

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

  • Add the --dont-fold option to disable folding a specific primop

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?

@seldridge seldridge changed the title Add --dont-fold option to disable folding prim ops Add --dont-fold option to disable folding specific prim ops Jan 16, 2021
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

Some pretty big nits, but otherwise this looks good!

src/main/scala/firrtl/transforms/ConstantPropagation.scala Outdated Show resolved Hide resolved
src/main/scala/firrtl/stage/FirrtlAnnotations.scala Outdated Show resolved Hide resolved
src/test/scala/firrtlTests/ConstantPropagationTests.scala Outdated Show resolved Hide resolved
This adds a --dont-fold options (backed by a DisableFold annotation)
that lets a user specify primitive operations which should never be
folded. This feature lets a user disable certain folds which may be
allowable in FIRRTL (or by any sane synthesis tool), but due to inane
Verilog language design causes formal equivalence tools to fail due to
the fold.

Add a test that a user can disable `a / a -> 1` with a
DisableFold(PrimOps.Div) annotation.

Signed-off-by: Schuyler Eldridge <[email protected]>
@jackkoenig jackkoenig added this to the 1.4.x milestone Jan 20, 2021
@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Jan 20, 2021
@mergify mergify bot merged commit 698a9dc into master Jan 20, 2021
mergify bot pushed a commit that referenced this pull request Jan 20, 2021
This adds a --dont-fold options (backed by a DisableFold annotation)
that lets a user specify primitive operations which should never be
folded. This feature lets a user disable certain folds which may be
allowable in FIRRTL (or by any sane synthesis tool), but due to inane
Verilog language design causes formal equivalence tools to fail due to
the fold.

Add a test that a user can disable `a / a -> 1` with a
DisableFold(PrimOps.Div) annotation.

Signed-off-by: Schuyler Eldridge <[email protected]>
(cherry picked from commit 698a9dc)
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Jan 20, 2021
@jackkoenig jackkoenig deleted the dev/seldridge/issue-2029 branch January 20, 2021 03:58
mergify bot added a commit that referenced this pull request Jan 20, 2021
This adds a --dont-fold options (backed by a DisableFold annotation)
that lets a user specify primitive operations which should never be
folded. This feature lets a user disable certain folds which may be
allowable in FIRRTL (or by any sane synthesis tool), but due to inane
Verilog language design causes formal equivalence tools to fail due to
the fold.

Add a test that a user can disable `a / a -> 1` with a
DisableFold(PrimOps.Div) annotation.

Signed-off-by: Schuyler Eldridge <[email protected]>
(cherry picked from commit 698a9dc)

Co-authored-by: Schuyler Eldridge <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Addition (no impact on existing code) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Option to Disable Div(a, a) -> 1 Fold
2 participants