fmt: remove lzero by lowering during Verilog parse

See https://github.com/YosysHQ/yosys/pull/3721#issuecomment-1502037466
-- this reduces logic within the cell, and makes the rules that apply
much more clear.

backends/cxxrtl/cxxrtl.h

@@ -802,11 +802,10 @@ struct value_formatted {
 	int width;
 	int base;
 	bool signed_;
-	bool lzero;
 	bool plus;
 
-	value_formatted(const value<Bits> &val, bool character, bool justify_left, char padding, int width, int base, bool signed_, bool lzero, bool plus) :
+	value_formatted(const value<Bits> &val, bool character, bool justify_left, char padding, int width, int base, bool signed_, bool plus) :
-		val(val), character(character), justify_left(justify_left), padding(padding), width(width), base(base), signed_(signed_), lzero(lzero), plus(plus) {}
+		val(val), character(character), justify_left(justify_left), padding(padding), width(width), base(base), signed_(signed_), plus(plus) {}
 	value_formatted(const value_formatted<Bits> &) = delete;
 	value_formatted<Bits> &operator=(const value_formatted<Bits> &rhs) = delete;
 };
@@ -823,7 +822,7 @@ std::ostream &operator<<(std::ostream &os, const value_formatted<Bits> &vf)
 
 	if (!vf.character) {
 		size_t width = Bits;
-		if (!vf.lzero && vf.base != 10) {
+		if (vf.base != 10) {
 			width = 0;
 			for (size_t index = 0; index < Bits; index++)
 				if (val.bit(index))

docs/source/CHAPTER_CellLib.rst

@@ -700,16 +700,12 @@ base
 	* ``c`` for ASCII characters/strings
 	* ``t`` and ``r`` for simulation time (corresponding to :verilog:`$time` and :verilog:`$realtime`)
 
-For integers, these items follow:
+For integers, this item may follow:
 
 ``+``\ *?*
 	(optional, decimals only) Include a leading plus for non-negative numbers.
 	This can assist with symmetry with negatives in tabulated output.
 
-``0``\ *?*
-	(optional, non-decimals only) Zero-pad the number to fit the signal's
-	largest value before any further padding/justification.
-
 signedness
 	``u`` for unsigned, ``s`` for signed.  This distinction is only respected
 	when rendering decimals.
@@ -730,8 +726,7 @@ Some example format specifiers:
   right-justified, zero-padded to 2 characters wide.
 + ``{32:< 15d+s}`` - 32-bit signed integer rendered as decimal, left-justified,
   space-padded to 15 characters wide, positive values prefixed with ``+``.
-+ ``{16:< 10h0u}`` - 16-bit unsigned integer rendered as hexadecimal,
++ ``{16:< 10hu}`` - 16-bit unsigned integer rendered as hexadecimal,
-  zero-padded to fit the largest signal value (4 characters for hex),
   left-justified, space-padded to 10 characters wide.
 + ``{0:>010t}`` - simulation time, right-justified, zero-padded to 10 characters
   wide.
@@ -742,6 +737,8 @@ and ``}}`` respectively.
 It is an error for a format string to consume more or less bits from ``\ARGS``
 than the port width.
 
+Values are never truncated, regardless of the specified width.
+
 Note that further restrictions on allowable combinations of options may apply
 depending on the backend used.
 

kernel/fmt.cc

@@ -130,12 +130,6 @@ void Fmt::parse_rtlil(const RTLIL::Cell *cell) {
 						log_assert(false && "Unexpected end in format substitution");
 				}
 
-				if (fmt[i] == '0') {
-					part.lzero = true;
-					if (++i == fmt.size())
-						log_assert(false && "Unexpected end in format substitution");
-				}
-
 				if (fmt[i] == 'u')
 					part.signed_ = false;
 				else if (fmt[i] == 's')
@@ -208,8 +202,6 @@ void Fmt::emit_rtlil(RTLIL::Cell *cell) const {
 					}
 					if (part.plus)
 						fmt += '+';
-					if (part.lzero)
-						fmt += '0';
 					fmt += part.signed_ ? 's' : 'u';
 				} else if (part.type == FmtPart::CHARACTER) {
 					fmt += 'c';
@@ -248,14 +240,72 @@ static size_t compute_required_decimal_places(size_t size, bool signed_)
 	return places;
 }
 
-static void apply_verilog_automatic_sizing(FmtPart &part)
+static size_t compute_required_nondecimal_places(size_t size, unsigned base)
+{
+	log_assert(base != 10);
+	BigUnsigned max;
+	max.setBit(size - 1, true);
+	size_t places = 0;
+	while (!max.isZero()) {
+		places++;
+		max /= base;
+	}
+	return places;
+}
+
+// Only called for integers, either when:
+//
+// (a) passed without a format string (e.g. "$display(a);"), or
+//
+// (b) the corresponding format specifier has no leading zero, e.g. "%b",
+// "%20h", "%-10d".
+//
+// In these cases, for binary/octal/hex, we always zero-pad to the size of the
+// signal; i.e. whether "%h" or "%10h" or "%-20h" is used, if the corresponding
+// signal is 32'h1234, "00001234" will always be a substring of the output.
+//
+// For case (a), we have no specified width, so there is nothing more to do.
+//
+// For case (b), because we are only called with no leading zero on the
+// specifier, any specified width beyond the signal size is therefore space
+// padding, whatever the justification.
+//
+// For decimal, we do no zero-padding, instead space-padding to the size
+// required for the signal's largest value.  This is per other Verilog
+// implementations, and intuitively makes sense as decimal representations lack
+// a discrete mapping of digits to bit groups.  Decimals may also show sign and
+// must accommodate this, whereas other representations do not.
+void Fmt::apply_verilog_automatic_sizing_and_add(FmtPart &part)
 {
 	if (part.base == 10) {
 		size_t places = compute_required_decimal_places(part.sig.size(), part.signed_);
 		part.padding = ' ';
 		part.width = std::max(part.width, places);
-	} else {
+		parts.push_back(part);
-		part.lzero = true;
+		return;
+	}
+
+	part.padding = '0';
+
+	size_t places = compute_required_nondecimal_places(part.sig.size(), part.base);
+	if (part.width < places) {
+		part.justify = FmtPart::RIGHT;
+		part.width = places;
+		parts.push_back(part);
+	} else if (part.width == places) {
+		parts.push_back(part);
+	} else if (part.width > places) {
+		auto gap = std::string(part.width - places, ' ');
+		part.width = places;
+
+		if (part.justify == FmtPart::RIGHT) {
+			append_string(gap);
+			parts.push_back(part);
+		} else {
+			part.justify = FmtPart::RIGHT;
+			parts.push_back(part);
+			append_string(gap);
+		}
 	}
 }
 
@@ -272,8 +322,7 @@ void Fmt::parse_verilog(const std::vector<VerilogFmtArg> &args, bool sformat_lik
 				part.sig = arg->sig;
 				part.base = default_base;
 				part.signed_ = arg->signed_;
-				apply_verilog_automatic_sizing(part);
+				apply_verilog_automatic_sizing_and_add(part);
-				parts.push_back(part);
 				break;
 			}
 
@@ -379,15 +428,13 @@ void Fmt::parse_verilog(const std::vector<VerilogFmtArg> &args, bool sformat_lik
 							if (part.padding == '\0')
 								part.padding = (has_leading_zero && part.justify == FmtPart::RIGHT) ? '0' : ' ';
 
-							if (part.type == FmtPart::INTEGER) {
+							if (part.type == FmtPart::INTEGER && part.base != 10 && part.plus)
-								if (part.base != 10 && part.plus) {
+								log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with invalid format specifier in argument %zu.\n", task_name.c_str(), fmtarg - args.begin() + 1);
-									log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with invalid format specifier in argument %zu.\n", task_name.c_str(), fmtarg - args.begin() + 1);
-								}
-								if (!has_leading_zero)
-									apply_verilog_automatic_sizing(part);
-							}
 
-							parts.push_back(part);
+							if (part.type == FmtPart::INTEGER && !has_leading_zero)
+								apply_verilog_automatic_sizing_and_add(part);
+							else
+								parts.push_back(part);
 							part = {};
 						}
 					}
@@ -439,13 +486,10 @@ std::vector<VerilogFmtArg> Fmt::emit_verilog() const
 				if (part.justify == FmtPart::LEFT)
 					fmt.str += '-';
 				if (part.width == 0) {
-					if (!part.lzero)
+					fmt.str += '0';
-						fmt.str += '0';
 				} else if (part.width > 0) {
 					log_assert(part.padding == ' ' || part.padding == '0');
-					if (!part.lzero && part.base != 10)
+					if (part.base != 10 || part.padding == '0')
-						fmt.str += '0';
-					else if (part.padding == '0')
 						fmt.str += '0';
 					fmt.str += std::to_string(part.width);
 				}
@@ -567,7 +611,6 @@ void Fmt::emit_cxxrtl(std::ostream &f, std::function<void(const RTLIL::SigSpec &
 				f << ", " << part.width;
 				f << ", " << part.base;
 				f << ", " << part.signed_;
-				f << ", " << part.lzero;
 				f << ", " << part.plus;
 				f << ')';
 				break;
@@ -584,7 +627,6 @@ void Fmt::emit_cxxrtl(std::ostream &f, std::function<void(const RTLIL::SigSpec &
 				f << ", " << part.width;
 				f << ", " << part.base;
 				f << ", " << part.signed_;
-				f << ", " << part.lzero;
 				f << ", " << part.plus;
 				f << ')';
 				break;
@@ -612,7 +654,7 @@ std::string Fmt::render() const
 				if (part.type == FmtPart::INTEGER) {
 					RTLIL::Const value = part.sig.as_const();
 
-					if (!part.lzero && part.base != 10) {
+					if (part.base != 10) {
 						size_t minimum_size = 0;
 						for (size_t index = 0; index < (size_t)value.size(); index++)
 							if (value[index] != State::S0)

kernel/fmt.h

@@ -75,7 +75,6 @@ struct FmtPart {
 	// INTEGER type
 	unsigned base = 10;
 	bool signed_ = false;
-	bool lzero = false;
 	bool plus = false;
 
 	// TIME type
@@ -83,6 +82,7 @@ struct FmtPart {
 };
 
 struct Fmt {
+public:
 	std::vector<FmtPart> parts;
 
 	void append_string(const std::string &str);
@@ -96,6 +96,9 @@ struct Fmt {
 	void emit_cxxrtl(std::ostream &f, std::function<void(const RTLIL::SigSpec &)> emit_sig) const;
 
 	std::string render() const;
+
+private:
+	void apply_verilog_automatic_sizing_and_add(FmtPart &part);
 };
 
 YOSYS_NAMESPACE_END