diff --git a/Changes b/Changes index 76ffd1dd8..10968aabc 100644 --- a/Changes +++ b/Changes @@ -15,6 +15,8 @@ indicates the contributor was also the author of the fix; Thanks! **** Fix missing VL_SHIFTRS_IQI with WIDTH warning, bug714. [Fabrizio Ferrandi] +**** Fix signed shift right optimization, bug715. [Fabrizio Ferrandi] + **** Fix internal error on "input x =" syntax error, bug716. [Lane Brooks] **** Fix slice extraction from packed array, bug717. [Jan Egil Ruud] diff --git a/include/verilated.h b/include/verilated.h index 892d0d397..364a8994f 100644 --- a/include/verilated.h +++ b/include/verilated.h @@ -1455,32 +1455,37 @@ static inline WDataOutP VL_SHIFTR_WWI(int obits,int,int,WDataOutP owp,WDataInP l } // EMIT_RULE: VL_SHIFTRS: oclean=false; lclean=clean, rclean==clean; -static inline IData VL_SHIFTRS_III(int obits, int, int, IData lhs, IData rhs) { +static inline IData VL_SHIFTRS_III(int obits, int lbits, int, IData lhs, IData rhs) { // Note the C standard does not specify the >> operator as a arithmetic shift! - IData sign = -(lhs >> (obits-1)); // ffff_ffff if negative - IData signext = ~(VL_MASK_I(obits) >> rhs); // One with bits where we've shifted "past" + // IEEE says signed if output signed, but bit position from lbits; + // must use lbits for sign; lbits might != obits, + // an EXTEND(SHIFTRS(...)) can became a SHIFTRS(...) within same 32/64 bit word length + IData sign = -(lhs >> (lbits-1)); // ffff_ffff if negative + IData signext = ~(VL_MASK_I(lbits) >> rhs); // One with bits where we've shifted "past" return (lhs >> rhs) | (sign & VL_CLEAN_II(obits,obits,signext)); } -static inline QData VL_SHIFTRS_QQI(int obits, int, int, QData lhs, IData rhs) { - QData sign = -(lhs >> (obits-1)); - QData signext = ~(VL_MASK_Q(obits) >> rhs); +static inline QData VL_SHIFTRS_QQI(int obits, int lbits, int, QData lhs, IData rhs) { + QData sign = -(lhs >> (lbits-1)); + QData signext = ~(VL_MASK_Q(lbits) >> rhs); return (lhs >> rhs) | (sign & VL_CLEAN_QQ(obits,obits,signext)); } static inline IData VL_SHIFTRS_IQI(int obits, int lbits, int rbits, QData lhs, IData rhs) { return (IData)(VL_SHIFTRS_QQI(obits, lbits, rbits, lhs, rhs)); } -static inline WDataOutP VL_SHIFTRS_WWI(int obits,int,int,WDataOutP owp,WDataInP lwp, IData rd) { +static inline WDataOutP VL_SHIFTRS_WWI(int obits,int lbits,int,WDataOutP owp,WDataInP lwp, IData rd) { int word_shift = VL_BITWORD_I(rd); int bit_shift = VL_BITBIT_I(rd); int lmsw = VL_WORDS_I(obits)-1; - IData sign = VL_SIGNONES_I(obits,lwp[lmsw]); - if ((int)rd >= obits) { + IData sign = VL_SIGNONES_I(lbits,lwp[lmsw]); + if ((int)rd >= obits) { // Shifting past end, sign in all of lbits for (int i=0; i <= lmsw; i++) owp[i] = sign; + owp[lmsw] &= VL_MASK_I(lbits); } else if (bit_shift==0) { // Aligned word shift (>>0,>>32,>>64 etc) int copy_words = (VL_WORDS_I(obits)-word_shift); for (int i=0; i < copy_words; i++) owp[i] = lwp[i+word_shift]; if (copy_words>=0) owp[copy_words-1] |= ~VL_MASK_I(obits) & sign; for (int i=copy_words; i < VL_WORDS_I(obits); i++) owp[i] = sign; + owp[lmsw] &= VL_MASK_I(lbits); } else { int loffset = rd & VL_SIZEBITS_I; int nbitsonright = 32-loffset; // bits that end up in lword (know loffset!=0) @@ -1495,6 +1500,7 @@ static inline WDataOutP VL_SHIFTRS_WWI(int obits,int,int,WDataOutP owp,WDataInP } if (words) owp[words-1] |= sign & ~VL_MASK_I(obits-loffset); for (int i=words; i output size, so don't want to force size }; struct AstShiftRS : public AstNodeBiop { + // Shift right with sign extension, >>> operator + // Output data type's width determines which bit is used for sign extension AstShiftRS(FileLine* fl, AstNode* lhsp, AstNode* rhsp, int setwidth=0) : AstNodeBiop(fl, lhsp, rhsp) { if (setwidth) { dtypeSetLogicSized(setwidth,setwidth,AstNumeric::SIGNED); } diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 1f4e517e6..e68c0dada 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -538,6 +538,8 @@ private: void replaceWChild(AstNode* nodep, AstNode* childp) { // NODE(..., CHILD(...)) -> CHILD(...) childp->unlinkFrBackWithNext(); + // If replacing a SEL for example, the data type comes from the parent (is less wide). + // This may adversly affect the operation of the node being replaced. childp->dtypeFrom(nodep); nodep->replaceWith(childp); nodep->deleteTree(); nodep=NULL; @@ -1831,7 +1833,7 @@ private: TREEOP1("AstGte {$lhsp.isAllOnes, $rhsp, $lhsp->width()==$rhsp->width()}", "replaceNumLimited(nodep,1)"); // Two level bubble pushing TREEOP ("AstNot {$lhsp.castNot, $lhsp->width()==$lhsp->castNot()->lhsp()->width()}", "replaceWChild(nodep, $lhsp->op1p())"); // NOT(NOT(x))->x - TREEOP ("AstLogNot{$lhsp.castLogNot}", "replaceWChild(nodep, $lhsp->op1p())"); // NOT(NOT(x))->x + TREEOP ("AstLogNot{$lhsp.castLogNot}", "replaceWChild(nodep, $lhsp->op1p())"); // LOGNOT(LOGNOT(x))->x TREEOPV("AstNot {$lhsp.castEqCase, $lhsp.width1}","AstNeqCase{$lhsp->op1p(),$lhsp->op2p()}"); TREEOP ("AstLogNot{$lhsp.castEqCase}", "AstNeqCase{$lhsp->op1p(),$lhsp->op2p()}"); TREEOPV("AstNot {$lhsp.castNeqCase, $lhsp.width1}","AstEqCase {$lhsp->op1p(),$lhsp->op2p()}");