chipsalliance / chipsalliance/chisel

Static Right shift of SInt is inconsistent: neither arithmetic nor logical

Open
#1,528 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Scala
Stars
4.8k
Forks
658
Avg merge
18h 59m
Merged PRs (30d)
14

Description

**Type of issue**: bug report

**Impact**: unknown

**Development Phase**: proposal

**If the current behavior is a bug, please provide the steps to reproduce the problem:**
```scala
class PlayGround() extends MultiIOModule {
val testU = IO(Output(UInt(32.W)))
val testS = IO(Output(SInt(32.W)))
val tmp = -8.S(32.W) >> 2
println(tmp)
testU := tmp.asUInt
testS := tmp
}
```
**What is the current behavior?**
Prints
```firrtl
SInt<30>(OpResult in PlayGround)
```
and produces
```verilog
assign testU = 32'h3ffffffe; // Logical Shift for the bits
assign testS = -32'sh2; // Arithmetic Shift for the value
```

**What is the expected behavior?**

Behaviours should be consistent, which can be solved in 2 ways

#### 1- Introduce an actual arithmetic shift operator >>> and have proper logical shift with >>
In that case >> operator will be an actual logical shift for both value and bits, with the following expected result
Prints
```firrtl
SInt<32>(OpResult in PlayGround)
```
and produces
```verilog
assign testU = 32'h3ffffffe; // Logical Shift for the bits
assign testS = 32'h3ffffffe; // Logical Shift for the value
```

#### 2- Always pad >>
Provide a proper arithmetic shift for the bits also, with actual sign extension
Prints
```firrtl
SInt<32>(OpResult in PlayGround)
```
and produces
```verilog
assign testU = 32'hfffffffe; // Arithmetic Shift for the bits
assign testS = -32'sh2; // Arithmetic Shift for the value
```

### >>> In both cases the width of shift result should be the same of original expression <<<

Note :

```scala
testU := tmp.pad(32).asUInt
```

is **not** an acceptable answer...

### EDIT
Just realised that this behavior is limited to static shift as dynamic shift cannot anticipate the maximum number of bits required to represent the the value.

There is definitely a **MAJOR** flaw here:
- inconsistent behavior between static and dynamic
- confusing and error prone
- misplaced "kind-of" chisel-elaboration-time "optimization" on width inference ...

### EDIT 2 - it is even worse because "dynamic" shift of UInt literal behaves as expected (so differently ... !!!!)
With
```scala
val tmp = -8.S(32.W) >> 2.U
```
it prints
```firrtl
SInt<32>(OpResult in PlayGround)
```
and produces
```verilog
assign testU = 32'hfffffffe; // Arithmetic Shift for the bits
assign testS = -32'sh2; // Arithmetic Shift for the value
```

this definitely needs to be fixed for consistency, this is a nightmare from user point of view ... !

**Please tell us about your environment:**
- version: `3.3.0`
- OS: `Darwin 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 18 21:21:34 PDT 2020; root:xnu-4570.71.82.5~1/RELEASE_X86_64 x86_64`

**What is the use case for changing the behavior?**
Well it's an awful and hidden bug ...

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.