kernel: treat zero width constant as zero

[georgerennie]

What are the reasons/motivation for this change?

Fixes #5175

Explain how this is achieved.

Zero width constants are special cased and treated as the constant value zero.

If applicable, please suggest to reviewers how they can test the change.

The testcase from #5175

[cr1901]

Can confirm this fixes Sentinel CI failures when I test this patch locally. I'll just have to wait until a yosys with this PR makes its way to OSS-CAD-Suite/yowasp before I can make CI green.

[georgerennie]

Maybe it would be useful to have a few amaranth designs in the test suite - verilog derived designs generally shouldn't end up with zero width cells.

[KrystalDelusion]

Treating a disconnected port (zero width or not) as a constant instead of undefined feels counter intuitive to me... Is there precedence for this elsewhere in Yosys, or is it just that it worked before so we shouldn't change it?

[georgerennie]

Yeah I am equally kinda uncomfortable with it which is why I had put an assertion guarding against it in there in the first place - to me it seemed like something that wouldn't be super well defined and should be avoided. That being said, it does seem that amaranth codegen seems to rely on this behaviour so we should support some behaviour for it.

The existing as_int infrastructure will treat it as zero, and after discussing with @widlarizer and others I think it is fair to define it as zero, that is the only value that can be represented in [0, 2^0). xs tend to have two meanings in Yosys, either as a don't care or more specifically as a Verilog x. Verilog xs (and signals in general) can't be zero width, so I think its reasonable to assume if we have a zero width signal it doesnt correspond to a Verilog x, and if we take it to mean don't care then the only concrete value that don't care can actually correspond to for that width is 0 so x and 0 are equivalent. Its worth noting that the cells this matters for are not just ones with undefined inputs, but specifically with a zero width - a disconnected port would normally be defined with 'x and a non-zero width instead afaik?

Also worth noting that this new try_as_int/as_int_saturating infrastructure is currently only being used in the shift optimization in opt_expr, so anywhere else in the codebase is still using as_int that treats it as zero.

I'm not sure any of this is 100% convincing, but after thinking about it for a bit it seems as reasonable approach as any to me and shouldnt change existing behaviour.

[KrystalDelusion]

after discussing with @widlarizer and others I think it is fair to define it as zero, that is the only value that can be represented in [0, 2^0)

Yeah actually that does seem reasonable

[whitequark]

Treating a disconnected port (zero width or not) as a constant instead of undefined feels counter intuitive to me...

I'm not sure I agree. A zero-width concatenation isn't the same as being disconnected, is it?

[KrystalDelusion]

connect \B { } is (also) used to describe a disconnected port

[whitequark]

connect \B { } is (also) used to describe a disconnected port

Oh, that's... cursed. I think this is an issue with the RTLIL design, but now that you bring it up I also think that Amaranth may be misusing RTLIL here.

[whitequark]

@KrystalDelusion On discussion, I think this is wrong. There aren't any port bits that aren't being connected here. The port is also zero width:

    parameter \B_WIDTH 0
    connect \B {  }

[whitequark]

Also, wouldn't a disconnected port just be missing from the connection list of a cell?

[KrystalDelusion]

Also, wouldn't a disconnected port just be missing from the connection list of a cell?

Nope

ERROR: Found error in internal cell \top.$85 ($shr) at kernel/rtlil.cc:1439:
  cell $shr $85
    parameter \Y_WIDTH 1
    parameter \B_WIDTH 0
    parameter \A_WIDTH 1
    parameter \B_SIGNED 0
    parameter \A_SIGNED 0
    connect \Y \Y
    connect \A \A
  end

[whitequark]

Nope

I don't see how your snippet suppots your conclusion. All it shows is that internal cells don't allow ports to be disconnected.

[KrystalDelusion]

I don't see how your snippet suppots your conclusion. All it shows is that internal cells don't allow ports to be disconnected.

The cell that started this PR/discussion was a $shr cell, I said that a connection to { } was the way a disconnected port was rendered in RTLIL, you suggested that a disconnected port could be missing from the list of connections rather than being connected to { }, I showed that a $shr cell (which is, once again, the cell that started this) missing a port connection was invalid RTLIL. The way that RTLIL describes a disconnected port (for internal cells and not) is by connecting them to { }:

yosys/backends/rtlil/rtlil_backend.cc

Lines 107 to 119 in a0a77cd

	void RTLIL_BACKEND::dump_sigspec(std::ostream &f, const RTLIL::SigSpec &sig, bool autoint)
	{
	if (sig.is_chunk()) {
	dump_sigchunk(f, sig.as_chunk(), autoint);
	} else {
	f << stringf("{ ");
	for (auto it = sig.chunks().rbegin(); it != sig.chunks().rend(); ++it) {
	dump_sigchunk(f, *it, false);
	f << stringf(" ");
	}
	f << stringf("}");
	}
	}

That's what gets called when dumping a cell.

[KrystalDelusion]

For what it's worth I was initially trying to show read_RTLIL of a cell with a missing port connection would be printed back out during write_RTLIL as connect { }, but I was mistaken and that isn't true for non-internal cells.

[whitequark]

For what it's worth I was initially trying to show read_RTLIL of a cell with a missing port connection would be printed back out during write_RTLIL as connect { }, but I was mistaken and that isn't true for non-internal cells.

Yes, that is what I'm trying to tell you.

[gatecat]

Friendly ping, has this been forgotten about?

[whitequark]

Disappointingly this wasn't a part of the last release..

[KrystalDelusion]

has this been forgotten about?

That's probably my fault. I was getting frustrated with the discussion and had to stop engaging.

[georgerennie]

Yeah I've been away a lot in the past few weeks and not particularly actively keeping track of things, I probably could have prodded it as well.