[MooreToCore] Properly deal with OOB access in dyn_extract

[maerhart]

The array slice case is not fixed yet. I couldn't really find a part in the SV standard about signed/unsigned indexing. I assume always interpreting the index operand as unsigned is fine.

[maerhart]

That can cause problems if the lowBit bitwidth is too small to hold the array size.

[fabianschuiki]

Nice catch. In that case we might be guaranteed to always access in bounds?

[maerhart]

Yes 👍
But I need to make sure this IR is not built in that case because the pass would crash.

[fabianschuiki]

LGTM!

[fabianschuiki]

Nice catch. In that case we might be guaranteed to always access in bounds?

[fabianschuiki]

This looks reasonable assuming that hw::ArrayGetOp has undefined behaviour (or undefined result) for out-of-bounds accesses. I don't remember off the top of my head whether out-of-bounds index in ArrayGetOp causes a potential segfault in the lowered LLVM IR… But it might be nice if hw::ArrayGetOp just produce an undefined result, but don't cause arbitrary undefined behaviour such as crashing the simulator. Then we can just do exactly what you did here: read the value and then discard it with a mux if it's out of bounds.

[maerhart]

The documentation of the HW aggregate ops is generally a bit sparse, but I'd say a OOB hw.array_get produces an undefined result, but no immediate UB. The HWToLLVM lowering currently does not take this into account. I'll create an issue for that.

[hailongSun2000]

I have a question about the unsigned index, for example:

logic [6:-1] arr;
arr[-1];

Maybe -1 represents the max if we assume the index is unsigned, right 🤔?

[fabianschuiki]

@hailongSun2000 I think in your example ImportVerilog would lower this to an access index 0, because -1 is the "address" of the least significant bit. During conversion, the accessed index gets remapped from the range [a:b] to [a-b:0]. 🙂

[hailongSun2000]

@hailongSun2000 I think in your example ImportVerilog would lower this to an access index 0, because -1 is the "address" of the least significant bit. During conversion, the accessed index gets remapped from the range [a:b] to [a-b:0]. 🙂

That sounds great! All bit rang are converted into little endian, and LSB is always 0. Could you tell me where the implementation is? I'm interested to it. Thanks in advance! 😃

[hailongSun2000]

LGTM! 🥳

[fabianschuiki]

@hailongSun2000 All bit rang are converted into little endian, and LSB is always 0. Could you tell me where the implementation is?

I think @maerhart added this getSelectIndex call here to deal with the index remapping in a PR a few days ago: 🙂

circt/lib/Conversion/ImportVerilog/Expressions.cpp

Line 480 in 6e93a0e

getSelectIndex(lowBit, range));

[maerhart]

As a little side-note: the example above would be lowered to an moore.extract, not a moore.dyn_extract

Even though all ranges are converted to little endian starting at 0, the index could still be a negative signed value in SV.

bit [31:0] arr;
bit [31:0] idx = -1;
arr[$signed(idx)];

What index does this actually access? -1 or 4,294,967,295? I couldn't really find an answer to this question in the standard, so let me know if you know more 😄
If both are out-of-bounds, it doesn't really matter, but could you have something like

bit [4294967295:0] arr; // let's say all bits are 1
bit [31:0] idx = -1;
arr[$signed(idx)];

Should this now return 0 or 1? Does SV even support such big arrays (if it doesn't the same still applies but with smaller integers)?

[fabianschuiki]

Since SV allows you to have negative indices on array declarations, like bit [3:-8] x, I'd assume that it supports signed indices when accessing arrays. So x[-2] would actually be treated as accessing index -2, which then gets translated to actual bit positions through that index remapping.

Since this only involves addition/subtraction which are identical in signed/unsigned arithmetic, I think we can just ignore the issue altogether: if you subtract the lower index of the declaration (-8) from the accessed index (-2), you'll automatically remap all valid negative indices to positive bit positions. Anything below the lowest bit address would overflow and be out of bounds immediately.

If I recall correctly, array index expressions have a self-determined type, not a type inferred from context. That would suggest in your example that the index $signed(idx) would still be a 32 bit number, even though this would create an ambiguity when accessing that large array. We could think about sign-extending signed indices in bit/part-select operations by one bit, to remove any ambiguity, or to sign/zero-extend the index to the minimum required to unambiguously address the entire array.