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

Fix signedness of xor const prop with zero #2179

Merged
merged 1 commit into from
Apr 16, 2021
Merged

Fix signedness of xor const prop with zero #2179

merged 1 commit into from
Apr 16, 2021

Conversation

fabianschuiki
Copy link
Contributor

Constant propagation of the Xor op folds xor(a, SInt(0)) to asUInt(a). For comparison, Or folds to asUInt(pad(a, W)). This can be a problem in the following case:

circuit Foo :
  module Foo :
    input a: UInt<3>
    output b: UInt<4>
    b <= xor(asSInt(a), SInt<4>(0))

This would emit the assignment as b = a instead of the sign-extended b = {{1{a[2]}},a}.

@CLAassistant
Copy link

CLAassistant commented Apr 15, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@fabianschuiki fabianschuiki mentioned this pull request Apr 15, 2021
Closed
seldridge
seldridge approved these changes Apr 15, 2021
View reviewed changes
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Looks great.

Can you add one test of this somewhere around here?

@fabianschuiki
Copy link
Contributor Author

Added a test and a few follow-up fixes that it uncovered.

@fabianschuiki
Copy link
Contributor Author

fabianschuiki commented Apr 15, 2021 •
edited
Loading

Love the scalafmt ❤️

seldridge
seldridge approved these changes Apr 15, 2021
View reviewed changes
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Nice!

One final nit about simplifying the test case. Actual functional code looks great.

src/test/scala/firrtlTests/ConstantPropagationTests.scala Outdated Show resolved Hide resolved
src/test/scala/firrtlTests/ConstantPropagationTests.scala Outdated Show resolved Hide resolved
@fabianschuiki
Fix signedness of xor const prop with zero
0d13a69 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.
@fabianschuiki
Copy link
Contributor Author

That seems to have fixed it. Ready from my side 🙂

@seldridge seldridge added this to the 1.2.x milestone Apr 16, 2021
@seldridge seldridge added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Apr 16, 2021
@mergify mergify bot merged commit e9b2946 into chipsalliance:master Apr 16, 2021
mergify bot pushed a commit that referenced this pull request Apr 16, 2021
@fabianschuiki
Fix signedness of xor const prop with zero (#2179)
d326c4f 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala
@mergify mergify bot mentioned this pull request Apr 16, 2021
Merged
mergify bot pushed a commit that referenced this pull request Apr 16, 2021
@fabianschuiki
Fix signedness of xor const prop with zero (#2179)
2b08435 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala
@mergify mergify bot mentioned this pull request Apr 16, 2021
Merged
mergify bot pushed a commit that referenced this pull request Apr 16, 2021
@fabianschuiki
Fix signedness of xor const prop with zero (#2179)
28ea1fc 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)
@mergify mergify bot mentioned this pull request Apr 16, 2021
Merged
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Apr 16, 2021
mergify bot added a commit that referenced this pull request Apr 16, 2021
@mergify @fabianschuiki
Fix signedness of xor const prop with zero (#2179) (#2185)
7dfb7a1 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

Co-authored-by: Fabian Schuiki <[email protected]>
fabianschuiki added a commit to fabianschuiki/firrtl that referenced this pull request Apr 16, 2021
@fabianschuiki
fixup! Fix signedness of xor const prop with zero (chipsalliance#2179)
a93aec9
fabianschuiki added a commit to fabianschuiki/firrtl that referenced this pull request Apr 16, 2021
@fabianschuiki
fixup! Fix signedness of xor const prop with zero (chipsalliance#2179)
0b561e1
fabianschuiki added a commit to fabianschuiki/firrtl that referenced this pull request Apr 16, 2021
@fabianschuiki
Fix signedness of xor const prop with zero (chipsalliance#2179)
3ebd19d 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala
fabianschuiki added a commit to fabianschuiki/firrtl that referenced this pull request Apr 16, 2021
@fabianschuiki
Fix signedness of xor const prop with zero (chipsalliance#2179)
be49ed2 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala
seldridge pushed a commit that referenced this pull request Apr 16, 2021
@fabianschuiki @seldridge
Fix signedness of xor const prop with zero (#2179)
6193f7b 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala
seldridge pushed a commit that referenced this pull request Apr 16, 2021
@fabianschuiki @seldridge
Fix signedness of xor const prop with zero (#2179)
e33026f 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala
mergify bot added a commit that referenced this pull request Apr 16, 2021
@mergify @fabianschuiki
Fix signedness of xor const prop with zero (#2179) (#2183)
674a1bb 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala

Co-authored-by: Fabian Schuiki <[email protected]>
mergify bot added a commit that referenced this pull request Apr 16, 2021
@mergify @fabianschuiki
Fix signedness of xor const prop with zero (#2179) (#2184)
2346100 
Constant propagation of the Xor op folds `xor(a, SInt(0))` to
`asUInt(a)`. For comparison, Or folds to `asUInt(pad(a, W))`. This can
be a problem in the following case:

    circuit Foo :
      module Foo :
        input a: UInt<3>
        output b: UInt<4>
        b <= asUInt(xor(asSInt(a), SInt<4>(0)))

This would emit the assignment as `b = a` instead of the sign-extended
`b = {{1{a[2]}},a}`.

This requires adjusting the `pad(e, t)` function use in const prop,
which currently just inserts a `Pad` prim op with the requested output
type. However, the function advertises that it pads *to the width* of
the type `t`. Some of the folds rely on this and request the padding of
a SInt<N> to the width of a UInt<M>. But the current implementation then
then actually returns a `Pad` op with type UInt<M>, instead of the
SInt<M> that was requested.

(cherry picked from commit e9b2946)

# Conflicts:
#	src/main/scala/firrtl/transforms/ConstantPropagation.scala

Co-authored-by: Fabian Schuiki <[email protected]>
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.
Projects
None yet
Milestone
1.2.x
Development

Successfully merging this pull request may close these issues.
3 participants
@fabianschuiki @CLAassistant @seldridge