Reject negative shift amounts; add tests (#730)

Closes #729

4 files changed, 28 insertions, 6 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
index 4df46b75..61617775 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
@@ -308,6 +308,12 @@ sealed abstract class Bits(width: Width, override val litArg: Option[LitArg])
 
   /** Default print as [[Decimal]] */
   final def toPrintable: Printable = Decimal(this)
+
+  protected final def validateShiftAmount(x: Int): Int = {
+    if (x < 0)
+      Builder.error(s"Negative shift amounts are illegal (got $x)")
+    x
+  }
 }
 
 // REVIEW TODO: Further discussion needed on what Num actually is.
@@ -494,13 +500,13 @@ sealed class UInt private[core] (width: Width, lit: Option[ULit] = None)
   def do_unary_! (implicit sourceInfo: SourceInfo, compileOptions: CompileOptions) : Bool = this === 0.U(1.W)
 
   override def do_<< (that: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): UInt =
-    binop(sourceInfo, UInt(this.width + that), ShiftLeftOp, that)
+    binop(sourceInfo, UInt(this.width + that), ShiftLeftOp, validateShiftAmount(that))
   override def do_<< (that: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): UInt =
     this << that.toInt
   override def do_<< (that: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): UInt =
     binop(sourceInfo, UInt(this.width.dynamicShiftLeft(that.width)), DynamicShiftLeftOp, that)
   override def do_>> (that: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): UInt =
-    binop(sourceInfo, UInt(this.width.shiftRight(that)), ShiftRightOp, that)
+    binop(sourceInfo, UInt(this.width.shiftRight(that)), ShiftRightOp, validateShiftAmount(that))
   override def do_>> (that: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): UInt =
     this >> that.toInt
   override def do_>> (that: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): UInt =
@@ -662,13 +668,13 @@ sealed class SInt private[core] (width: Width, lit: Option[SLit] = None)
   }
 
   override def do_<< (that: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): SInt =
-    binop(sourceInfo, SInt(this.width + that), ShiftLeftOp, that)
+    binop(sourceInfo, SInt(this.width + that), ShiftLeftOp, validateShiftAmount(that))
   override def do_<< (that: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): SInt =
     this << that.toInt
   override def do_<< (that: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): SInt =
     binop(sourceInfo, SInt(this.width.dynamicShiftLeft(that.width)), DynamicShiftLeftOp, that)
   override def do_>> (that: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): SInt =
-    binop(sourceInfo, SInt(this.width.shiftRight(that)), ShiftRightOp, that)
+    binop(sourceInfo, SInt(this.width.shiftRight(that)), ShiftRightOp, validateShiftAmount(that))
   override def do_>> (that: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): SInt =
     this >> that.toInt
   override def do_>> (that: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): SInt =
@@ -924,13 +930,13 @@ sealed class FixedPoint private (width: Width, val binaryPoint: BinaryPoint, lit
   }
 
   override def do_<< (that: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): FixedPoint =
-    binop(sourceInfo, FixedPoint(this.width + that, this.binaryPoint), ShiftLeftOp, that)
+    binop(sourceInfo, FixedPoint(this.width + that, this.binaryPoint), ShiftLeftOp, validateShiftAmount(that))
   override def do_<< (that: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): FixedPoint =
     (this << that.toInt).asFixedPoint(this.binaryPoint)
   override def do_<< (that: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): FixedPoint =
     binop(sourceInfo, FixedPoint(this.width.dynamicShiftLeft(that.width), this.binaryPoint), DynamicShiftLeftOp, that)
   override def do_>> (that: Int)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): FixedPoint =
-    binop(sourceInfo, FixedPoint(this.width.shiftRight(that), this.binaryPoint), ShiftRightOp, that)
+    binop(sourceInfo, FixedPoint(this.width.shiftRight(that), this.binaryPoint), ShiftRightOp, validateShiftAmount(that))
   override def do_>> (that: BigInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): FixedPoint =
     (this >> that.toInt).asFixedPoint(this.binaryPoint)
   override def do_>> (that: UInt)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): FixedPoint =
diff --git a/src/test/scala/chiselTests/FixedPointSpec.scala b/src/test/scala/chiselTests/FixedPointSpec.scala
index 8caa7f1e..ff4b42a0 100644
--- a/src/test/scala/chiselTests/FixedPointSpec.scala
+++ b/src/test/scala/chiselTests/FixedPointSpec.scala
@@ -126,4 +126,7 @@ class FixedPointSpec extends ChiselPropSpec {
   property("should mux different widths and binary points") {
     assertTesterPasses { new FixedPointMuxTester }
   }
+  property("Negative shift amounts are invalid") {
+    a [ChiselException] should be thrownBy { elaborate(new NegativeShift(FixedPoint(1.W, 0.BP))) }
+  }
 }
diff --git a/src/test/scala/chiselTests/SIntOps.scala b/src/test/scala/chiselTests/SIntOps.scala
index 900eb074..a9b75446 100644
--- a/src/test/scala/chiselTests/SIntOps.scala
+++ b/src/test/scala/chiselTests/SIntOps.scala
@@ -89,5 +89,9 @@ class SIntOpsSpec extends ChiselPropSpec {
     elaborate { new SIntOps }
   }
 
+  property("Negative shift amounts are invalid") {
+    a [ChiselException] should be thrownBy { elaborate(new NegativeShift(SInt())) }
+  }
+
   ignore("SIntOpsTester should return the correct result") { }
 }
diff --git a/src/test/scala/chiselTests/UIntOps.scala b/src/test/scala/chiselTests/UIntOps.scala
index d31d86a8..490af22b 100644
--- a/src/test/scala/chiselTests/UIntOps.scala
+++ b/src/test/scala/chiselTests/UIntOps.scala
@@ -91,6 +91,11 @@ class BadBoolConversion extends Module {
   io.b := io.u.toBool
 }
 
+class NegativeShift(t: => Bits) extends Module {
+  val io = IO(new Bundle)
+  Reg(t) >> -1
+}
+
 class UIntOpsSpec extends ChiselPropSpec with Matchers {
   // Disable shrinking on error.
   implicit val noShrinkListVal = Shrink[List[Int]](_ => Stream.empty)
@@ -111,5 +116,9 @@ class UIntOpsSpec extends ChiselPropSpec with Matchers {
   property("UIntOpsTester should return the correct result") {
     assertTesterPasses { new UIntOpsTester(123, 7) }
   }
+
+  property("Negative shift amounts are invalid") {
+    a [ChiselException] should be thrownBy { elaborate(new NegativeShift(UInt())) }
+  }
 }