[Comb] Officialize support for zero-width integers

[Moxinilian]

This PR makes the undocumented support of i0 in comb official by removing the documentation stating it is not supported and adding a few tests to the canonicalizer (and marginally to the lowering to SV which has already been testing it for the most part).

Associated RFC

[darthscsi]

Given the change, I would make this explicitly positive. "Zero-bit integer width is supported" with notes on how i0 is handled and how ops which break on 0 are handled. Although not a comb issuer per-se, noting what is expected in export verilog is helpful too. Also perhaps notes that zero in types is structurally significant.

[Moxinilian]

I added a comment on comb. The behavior of the lowering to SV is actually already described in the HW rationale, I figured it's probably better there!

[Moxinilian]

I also adjusted the verifier for the replicate op so it is more consistent with the mathematical definition of a multiple.

[fabianschuiki]

I'm wondering if this is the behavior you'd expect. Intuitively you can replace comb.extract %a from 0 : (T) -> T with just %a, since the extract is a no-op. That would suggest you can do the same for T = i0, which makes this error somewhat unintuitive. I think comb.extract should be valid as you've written it. Maybe we should instead check if lowBit >= 0 and lowBit + resultWidth <= inputWidth, which would allow this case, and also comb.extract %a from 42 : (i42) -> i0.

[Moxinilian]

Do we want to allow comb.extract %a from 100000 : (i16)-> i0 then? Because conceptually it is the same kind of extraction outside the bounds of the integer. Intuitively it is taking bits where the original value has none, which is only fine if you are taking zero bits.

[maerhart]

Let me try to approach this from a more mathematical perspective: Let's consider the operation comb.extract %a from x : (iy) -> iz where x, y, z are integers >= 0. Let's decompose the input type to the interval of bit indices I = [0,y), and the output type to bit indices O = [x,x+z). Then, the minimal constraint we need for the op to work is O \subset I (otherwise, we'd need to define what should be returned for non i0 OOB accesses). If z = 0, then O is the empty set/interval, which is a subset of any other set; thus, it wouldn't matter what x and y are for the operation to be valid (this is what @Moxinilian points out). In the current implementation, we have the additional constraint that x \element I which cannot be satisfied if y = 0 since I is the empty set. @fabianschuiki is suggesting that this constraint should be changed to x \element (I \union [y, y]).

I think we shouldn't drop the second constraint entirely because I can't see a case where comb.extract %a from 100000 : (i16)-> i0 would ever be useful but is more likely a symptom of a bug somewhere that can be caught by the second constraint. In the end, it's a question of definition, and which one we choose is a question of what is more practical to work with in compiler passes. I suppose the extra flexibility @fabianschuiki proposes can be useful when working with module parameters (e.g., if a parameter N is used to extract the N LSB/MSB from an integer).

[Moxinilian]

It seems to be like a good compromise yes. I will update this as soon as possible.

[darthscsi]

comb.extract %a from 100000 : (i16)-> i0 should be illegal. It is very reasonable that in the IR, indexes are legal. If a front end language wants to have operations which allow "over-the-limit" extractions (like firrtl does), that's fine, but that complexity shouldn't impact comb.

The alternate argument is that parameters will make this hard to enforce.

[darthscsi]

Or put alternately, ops should not encode an implicit extension. So if comb.extract %a from 100000 : (i16)-> i0 is legal, that implies the op always implicitly extends the argument sufficiently to perform the extraction. This makes analysis much harder. Lots of bugs in FIRRTL come from places where people forgot that the ops have implicit extension and we are slowly reworking the dialect to rectify that.

[Moxinilian]

Extracting an i0 from bit 10000 is not an implicit extension though. The property "all extracted bits are defined" is satisfied even when no extension is carried out. But in any case, the verifier I implemented will not accept it, at least for now.

[Moxinilian]

@maerhart @fabianschuiki I have also documented it so people can come and remove the constraint if they can provide a situation where they need it.

[maerhart]

Suggested change

	can be infered for signed operations. In particular, division on `i0` is
	can be inferred for signed operations. In particular, division on `i0` is

[maerhart]

Maybe add a link to the division by zero section? It might also be great to document whether this other section implies that division by i0 is well-defined or not because, to me, 0 seems to be the only value it can be.