Fix bit reductions on multi-packed dimensions, bug227

Signed-off-by: Wilson Snyder <wsnyder@wsnyder.org>

Changes

@@ -13,6 +13,8 @@ indicates the contributor was also the author of the fix; Thanks!
 
 **** Report errors when extra underscores used in meta-comments.
 
+**** Fix bit reductions on multi-packed dimensions, bug227. [by Bryon Bradley]
+
 **** Fix "make install" with configure outside srcdir.  [Stefan Wallentowitz]
 
 **** Fix trace files with empty modules crashing some viewers.

src/V3Slice.cpp

@@ -54,6 +54,14 @@ class SliceCloneVisitor : public AstNVisitor {
     //  AstArraySel::user1p()	    -> AstVarRef. The VarRef that the final ArraySel points to
     //  AstNodeAssign::user2()	    -> int. The number of clones needed for this assign
 
+    // ENUMS
+    enum RedOp {		// The type of unary operation to be expanded
+	REDOP_UNKNOWN,		// Unknown/Unsupported
+	REDOP_OR,		// Or Reduction
+	REDOP_AND,		// And Reduction
+	REDOP_XOR,		// Xor Reduction
+	REDOP_XNOR};		// Xnor Reduction
+
     // STATE
     vector<vector<unsigned> >	m_selBits;	// Indexes of the ArraySel we are expanding
     int				m_vecIdx;	// Current vector index
@@ -78,7 +86,7 @@ class SliceCloneVisitor : public AstNVisitor {
 		AstVar* varp = m_refp->varp();
 		pair<uint32_t,uint32_t> arrDim = varp->dimensions();
 		uint32_t dimensions = arrDim.first + arrDim.second;
-		for (int i = 0; i < dimensions; ++i) {
+		for (uint32_t i = 0; i < dimensions; ++i) {
 		    m_selBits[m_vecIdx].push_back(0);
 		}
 	    }
@@ -113,6 +121,7 @@ class SliceCloneVisitor : public AstNVisitor {
     }
 
     virtual void visit(AstNodeAssign* nodep, AstNUser*) {
+	if (nodep->user2() < 2) return; // Don't need clones
 	m_selBits.clear();
 	UINFO(4, "Cloning "<<nodep->user2()<<" times: "<<nodep<<endl);
 	for (int i = 0; i < nodep->user2(); ++i) {
@@ -125,13 +134,60 @@ class SliceCloneVisitor : public AstNVisitor {
 	nodep->unlinkFrBack()->deleteTree(); nodep = NULL;
     }
 
+    virtual void visit(AstNodeUniop* nodep, AstNUser*) {
+	if (nodep->user2() < 2) return; // Don't need clones
+	m_selBits.clear();
+	UINFO(4, "Cloning "<<nodep->user2()<<" times: "<<nodep<<endl);
+
+	// Figure out what type of operation this is so we don't have to cast on
+	// every clone.
+	RedOp redOpType = REDOP_UNKNOWN;
+	if (nodep->castRedOr()) redOpType = REDOP_OR;
+	else if (nodep->castRedAnd()) redOpType = REDOP_AND;
+	else if (nodep->castRedXor()) redOpType = REDOP_XOR;
+	else if (nodep->castRedXnor()) redOpType = REDOP_XNOR;
+
+	AstNode* lhsp = NULL;
+	AstNode* rhsp = NULL;
+	for (int i = 0; i < nodep->user2(); ++i) {
+	    // Clone the node and iterate over the clone
+	    m_vecIdx = -1;
+	    AstNodeUniop* clonep = nodep->cloneTree(false)->castNodeUniop();
+	    clonep->iterateChildren(*this);
+	    if (!lhsp) lhsp = clonep;
+	    else rhsp = clonep;
+	    if (lhsp && rhsp) {
+		switch (redOpType) {
+		case REDOP_OR:
+		    lhsp = new AstLogOr(nodep->fileline(), lhsp, rhsp);
+		    break;
+		case REDOP_AND:
+		    lhsp = new AstLogAnd(nodep->fileline(), lhsp, rhsp);
+		    break;
+		case REDOP_XOR:
+		    lhsp = new AstXor(nodep->fileline(), lhsp, rhsp);
+		    break;
+		case REDOP_XNOR:
+		    lhsp = new AstXnor(nodep->fileline(), lhsp, rhsp);
+		    break;
+		default: // REDOP_UNKNOWN
+		    nodep->v3fatalSrc("Unsupported: Unary operation on multiple packed dimensions");
+		    break;
+		}
+		rhsp = NULL;
+	    }
+	}
+	nodep->addNextHere(lhsp);
+	nodep->unlinkFrBack()->deleteTree(); nodep = NULL;
+    }
+
     virtual void visit(AstNode* nodep, AstNUser*) {
 	// Default: Just iterate
 	nodep->iterateChildren(*this);
     }
 public:
     // CONSTUCTORS
-    SliceCloneVisitor(AstNodeAssign* assignp) {
+    SliceCloneVisitor(AstNode* assignp) {
 	assignp->accept(*this);
     }
     virtual ~SliceCloneVisitor() {}
@@ -143,11 +199,15 @@ class SliceVisitor : public AstNVisitor {
     // NODE STATE
     // Cleared on netlist
     //  AstNodeAssign::user1()	    -> bool.  True if find is complete
-    //  AstNodeAssign::user2()	    -> int. The number of clones needed for this assign
+    //  AstUniop::user1()	    -> bool.  True if find is complete
     //  AstArraySel::user1p()	    -> AstVarRef. The VarRef that the final ArraySel points to
+    //  AstNode::user2()	    -> int. The number of clones needed for this node
     AstUser1InUse	m_inuser1;
     AstUser2InUse	m_inuser2;
 
+    // TYPEDEFS
+    typedef pair<uint32_t, uint32_t> ArrayDimensions;	// Array Dimensions (packed, unpacked)
+
     // STATE
     AstNode*		m_assignp;	// Assignment we are under
     AstNodeVarRef*	m_lhsVarRefp;	// Var on the LHS
@@ -180,15 +240,13 @@ class SliceVisitor : public AstNVisitor {
 	return dim;
     }
 
-    AstNode* insertImplicit(AstVarRef* nodep, unsigned start, unsigned count) {
+    AstArraySel* insertImplicit(AstNode* nodep, unsigned start, unsigned count) {
 	// Insert any implicit slices as explicit slices (ArraySel nodes).
-	// Return a new pointer to replace fromp() in the ArraySel.
+	// Return a new pointer to replace nodep() in the ArraySel.
-	AstVarRef* fromp = nodep;
+	AstVarRef* refp = nodep->user1p()->castNode()->castVarRef();
-	if (!fromp) nodep->v3fatalSrc("NULL VarRef passed to insertImplicit");
+	if (!refp) nodep->v3fatalSrc("No VarRef in user1 of node "<<nodep);
-	AstVar* varp = fromp->varp();
+	AstVar* varp = refp->varp();
-	// Get the DType and insert a new ArraySel
+	AstNode* topp = nodep;
-	AstArraySel* topp = NULL;
-	AstArraySel* bottomp = NULL;
 	for (unsigned i = start; i < start + count; ++i) {
 	    AstNodeDType* dtypep = varp->dtypeDimensionp(i-1);
 	    AstArrayDType* adtypep = dtypep->castArrayDType();
@@ -199,17 +257,13 @@ class SliceVisitor : public AstNVisitor {
 		// Below code assumes big bit endian; just works out if we swap
 		int x = msb; msb = lsb; lsb = x;
 	    }
-	    AstArraySel* newp = new AstArraySel(nodep->fileline(), fromp, new AstConst(nodep->fileline(),lsb));
+	    AstArraySel* newp = new AstArraySel(nodep->fileline(), topp, new AstConst(nodep->fileline(),lsb));
+	    newp->user1p(refp);
 	    newp->start(lsb);
 	    newp->length(msb - lsb + 1);
-	    if (!topp) topp = newp;
+	    topp = newp->castNode();
-	    fromp = newp->fromp()->unlinkFrBack()->castVarRef();
-
-	    if (bottomp) bottomp->fromp(newp);
-	    bottomp = newp;
 	}
-	bottomp->fromp(fromp);
+	return topp->castArraySel();
-	return topp;
     }
 
     int countClones(AstArraySel* nodep) {
@@ -233,7 +287,9 @@ class SliceVisitor : public AstNVisitor {
 	    pair<uint32_t,uint32_t> arrDim = nodep->varp()->dimensions();
 	    uint32_t dimensions = arrDim.first + arrDim.second;
 	    if (dimensions > 0) {
-		AstNode* newp = insertImplicit(nodep->cloneTree(false), 1, dimensions);
+		AstVarRef* clonep = nodep->cloneTree(false);
+		clonep->user1p(nodep);
+		AstNode* newp = insertImplicit(clonep, 1, dimensions);
 		nodep->replaceWith(newp); nodep = NULL;
 		newp->accept(*this);
 	    }
@@ -265,9 +321,8 @@ class SliceVisitor : public AstNVisitor {
 	pair<uint32_t,uint32_t> arrDim = refp->varp()->dimensions();
 	uint32_t implicit = (arrDim.first + arrDim.second) - dim;
 	if (implicit > 0) {
-	    AstNode* backp = refp->backp();
+	    AstArraySel* newp = insertImplicit(nodep->cloneTree(false), dim+1, implicit);
-	    AstNode* newp = insertImplicit(refp->cloneTree(false), dim+1, implicit);
+	    nodep->replaceWith(newp); nodep = newp;
-	    backp->castArraySel()->fromp()->replaceWith(newp);
 	}
 	int clones = countClones(nodep);
 	if (m_assignp->user2() > 0 && m_assignp->user2() != clones) {
@@ -278,7 +333,7 @@ class SliceVisitor : public AstNVisitor {
 		m_assignError = true;
 	    }
 	    if (clones > 1 && !refp->lvalue() && refp->varp() == m_lhsVarRefp->varp() && !m_assignp->castAssignDly()) {
-	        // LHS Var != RHS Var for a non-delayed assignment
+		// LHS Var != RHS Var for a non-delayed assignment
 		m_assignp->v3error("Unsupported: Slices in a non-delayed assignment with the same Var on both sides");
 		m_assignError = true;
 	    }
@@ -340,9 +395,7 @@ class SliceVisitor : public AstNVisitor {
 	// Iterate children looking for ArraySel nodes. From that we get the number of elements
 	// in the array so we know how many times we need to clone this assignment.
 	nodep->iterateChildren(*this);
-	if (nodep->user2() > 1) {
+	if (nodep->user2() > 1) SliceCloneVisitor scv(nodep);
-	    SliceCloneVisitor scv(nodep);
-	}
 	m_assignp = NULL;
     }
 
@@ -353,6 +406,60 @@ class SliceVisitor : public AstNVisitor {
 	}
     }
 
+    void expandUniOp(AstNodeUniop* nodep) {
+	nodep->user1(true);
+	unsigned dim = 0;
+	if (AstArraySel* selp = nodep->lhsp()->castArraySel()) {
+	    // We have explicit dimensions, either packed or unpacked
+	    dim = explicitDimensions(selp);
+	}
+	if (dim == 0 && !nodep->lhsp()->castVarRef()) {
+	    // No ArraySel or VarRef, not something we can expand
+	    nodep->iterateChildren(*this);
+	} else {
+	    AstVarRef* refp = findVarRefRecurse(nodep->lhsp());
+	    ArrayDimensions varDim = refp->varp()->dimensions();
+	    if ((int)(dim - varDim.second) < 0) {
+		// Unpacked dimensions are referenced first, make sure we have them all
+		nodep->v3error("Unary operator used across unpacked dimensions");
+	    } else if ((int)(dim - (varDim.first + varDim.second)) < 0) {
+		// Implicit packed dimensions are allowed, make them explicit
+		uint32_t newDim = (varDim.first + varDim.second) - dim;
+		AstNode* clonep = nodep->lhsp()->cloneTree(false);
+		clonep->user1p(refp);
+		AstNode* newp = insertImplicit(clonep, dim+1, newDim);
+		nodep->lhsp()->replaceWith(newp); refp = NULL;
+		int clones = countClones(nodep->lhsp()->castArraySel());
+		nodep->user2(clones);
+		SliceCloneVisitor scv(nodep);
+	    }
+	}
+    }
+
+    virtual void visit(AstRedOr* nodep, AstNUser*) {
+	if (!nodep->user1()) {
+	    expandUniOp(nodep);
+	}
+    }
+
+    virtual void visit(AstRedAnd* nodep, AstNUser*) {
+	if (!nodep->user1()) {
+	    expandUniOp(nodep);
+	}
+    }
+
+    virtual void visit(AstRedXor* nodep, AstNUser*) {
+	if (!nodep->user1()) {
+	    expandUniOp(nodep);
+	}
+    }
+
+    virtual void visit(AstRedXnor* nodep, AstNUser*) {
+	if (!nodep->user1()) {
+	    expandUniOp(nodep);
+	}
+    }
+
     virtual void visit(AstNode* nodep, AstNUser*) {
 	// Default: Just iterate
 	nodep->iterateChildren(*this);
@@ -372,6 +479,6 @@ public:
 // Link class functions
 
 void V3Slice::sliceAll(AstNetlist* rootp) {
-    UINFO(4,__FUNCTION__<<": "<<endl);
+    UINFO(2,__FUNCTION__<<": "<<endl);
     SliceVisitor visitor(rootp);
 }

test_regress/t/t_mem_packed.v

@@ -91,4 +91,78 @@ module t (/*AUTOARG*/
       end
    end
 
+   // Test for mixed implicit/explicit dimensions and all implicit packed
+   logic [3:0][7:0][1:0] vld [1:0][1:0];
+   logic [3:0][7:0][1:0] vld2;
+
+   // There are specific nodes for Or, Xor, Xnor and And
+   logic            vld_or;
+   logic            vld2_or;
+   assign vld_or = |vld[0][0];
+   assign vld2_or = |vld2;
+
+   logic            vld_xor;
+   logic            vld2_xor;
+   assign vld_xor = ^vld[0][0];
+   assign vld2_xor = ^vld2;
+
+   logic            vld_xnor;
+   logic            vld2_xnor;
+   assign vld_xnor = ~^vld[0][0];
+   assign vld2_xnor = ~^vld2;
+
+   logic            vld_and;
+   logic            vld2_and;
+   assign vld_and = &vld[0][0];
+   assign vld2_and = &vld2;
+
+   // Test an AstNodeUniop that shouldn't be expanded
+   logic [3:0][7:0][1:0] vld2_inv;
+   assign vld2_inv = ~vld2;
+
+   initial begin
+      for (int i=0; i<4; i=i+2) begin
+         for (int j=0; j<8; j=j+2) begin
+	    vld[0][0][i][j] = 2'b00;
+	    vld[0][0][i+1][j+1] = 2'b00;
+	    vld2[i][j] = 2'b00;
+	    vld2[i+1][j+1] = 2'b00;
+	 end
+      end
+   end
+
+   logic [3:0] expect_cyc; initial expect_cyc = 'd15;
+
+   always @(posedge clk) begin
+      expect_cyc <= expect_cyc + 1;
+      for (int i=0; i<4; i=i+1) begin
+         for (int j=0; j<8; j=j+1) begin
+	    vld[0][0][i][j] <= vld[0][0][i][j] + 1;
+	    vld2[i][j] <= vld2[i][j] + 1;
+	 end
+      end
+      if (cyc % 8 == 0) begin
+	 vld[0][0][0][0] <= vld[0][0][0][0] - 1;
+	 vld2[0][0] <= vld2[0][0] - 1;
+      end
+      if (expect_cyc < 8 && !vld_xor) $stop;
+      else if (expect_cyc > 7 && vld_xor) $stop;
+
+      if (expect_cyc < 8 && vld_xnor) $stop;
+      else if (expect_cyc > 7 && !vld_xnor) $stop;
+
+      if (expect_cyc == 15 && vld_or) $stop;
+      else if (expect_cyc == 11 && vld_or) $stop;
+      else if (expect_cyc != 15 && expect_cyc != 11 && !vld_or) $stop;
+
+      if (expect_cyc == 10 && !vld_and) $stop;
+      else if (expect_cyc == 14 && !vld_and) $stop;
+      else if (expect_cyc != 10 && expect_cyc != 14 && vld_and) $stop;
+
+      if (vld_xor != vld2_xor) $stop;
+      if (vld_xnor != vld2_xnor) $stop;
+      if (vld_or != vld2_or) $stop;
+      if (vld_and != vld2_and) $stop;
+   end
+
 endmodule