Generalized array support

[ChrisDodd]

First change relaxes frontend checks to allow arrays in general as per p4lang/p4-spec#1320. Non-header arrays only support indexing, not any other header stack operations

Second change adds support for C-style array declarators, with the array size after the name instead of before it.

[jafingerhut]

Nice! The test cases look interesting. As usual (unforunately), I haven't attempted to review the C++ implementation code.

What is the behavior if you attempt to use an index expression that is not a compile-time known value?

I could imagine trying to allow that if the index expression type was bit<W>, and the array had size at least 2^W, since you can pretty easily statically guarantee that the index is in range in that case. Anything fancier than that and it starts to go down the rabbit hole of what can be proved about the range of values at compile time.

[ChrisDodd]

What is the behavior if you attempt to use an index expression that is not a compile-time known value?

I could imagine trying to allow that if the index expression type was bit<W>, and the array had size at least 2^W, since you can pretty easily statically guarantee that the index is in range in that case. Anything fancier than that and it starts to go down the rabbit hole of what can be proved about the range of values at compile time.

The frontend/midend won't do anything about it, but I would imaging that many targets would be unable to support non-constant indexes and the backend would reject it. It is not clear what a target that could support non-constant indexes could/should do about out of range indexes. In all of this, an array is the same as a header stack. This really just makes header stacks a special case of arrays -- they support some additional functions that arrays of non-header/header union types do not.

[kfcripps]

Could you please add positive and/or error test cases similar to the following to demonstrate what is expected when the array type is an array type or contains array types?

struct S {
    bit<16> f1;
    bit<8>[16] nested_arr;
    bit<16> f2;
}

header data_t {
    bit<32>     f1;
    bit<32>     f2;
    bit<16>     h1;
    bit<16>     h2;
    S[16]  arr;
}
...
    hdr.data.arr[x].nested_arr[y] = ...;
    ... = hdr.data.arr[z].nested_arr[w];
...

and (if the grammar allows it (I did not look very closely at changes to p4parser.ypp)):

header data_t {
    bit<32>     f1;
    bit<32>     f2;
    bit<16>     h1;
    bit<16>     h2;
    bit<8>[16][16]  arr;
}
...
    hdr.data.arr[x][y] = ...;
    ... = hdr.data.arr[z][w];
...

[ChrisDodd]

While these are allowed, perhaps they shouldn't be. They end up running extremely slowly, as the HSIndexSimplifier pass (which replaces non-constant indexes with if trees) winds up replcating this assignment 65536 times (all the possible combinations of indexes for x, y, z, and w) with as many ifs. Not terribly practical.

[kfcripps]

Explicitly disallowing nested arrays is completely fine with me (as long as it's made clear in the spec).

[kfcripps]

Should we also test the other case I mentioned (i.e.

header data_t {
    bit<32>     f1;
    bit<32>     f2;
    bit<16>     h1;
    bit<16>     h2;
    bit<8>[16][16]  arr;
}
...
    hdr.data.arr[x][y] = ...;
    ... = hdr.data.arr[z][w];
...

) (assuming that the grammar allows it)?

[ChrisDodd]

This code blows up too much. Even with only 2 indexes in a single statement, it gets 256 variants which expands the memory a lot and takes 5-10 minutes to run the test. Trying to do 3+ really blows up.

There are probably things that could be improved in HSIndexSimplifier pass to not blow up quite as badly, but it is tricky and probably beyond the scope of this change.

[kfcripps]

Is there a reason we do not want to explicitly disallow this case (by detecting it and emitting an error message)? Is it because not all backends run HSIndexSimplifier? If that's the case, then why not detect it inside of HSIndexSimplifier pass, before HSIndexContretizer (and exit with an error instead of running HSIndexContretizer if it is detected)?

[ChrisDodd]

The same problem can happen even without 2D arrays if they are large enough. The best "quick-fix" for HSIndexSimplifier is probably to put in a limit to the number of branch leaves inserted at 100 or so -- that would avoid any case that had a huge blowup independent of dimensions and such,

[ChrisDodd]

Ugh, HSIndexSimplifier is just broken in how it deals with multiple indexes on arrays/header stacks. It basically goes over things multiple times, hoping everything gets expanded, which may or may not happen (and may create many unneeded duplicates of things).

[ChrisDodd]

I've add a limit to the expansion in HSIndexSimplifier -- it now tracks how many additional things it is adding and errors out at an arch-specified limit (defaults to 1000).

I don't like the code of this pass -- it is pretty convoluted and probably broken for some corner cases we don't check. Should be replaced by a simpler pass that does not create and apply sub-passes dynamically and does a better job.

[jonathan-dilorenzo]

Takeaways from LDWG:

• Would be great to have the property that these arrays can be desugared to basic primitives ideally in a pass in the frontend or the midend. This could guide us on the types of arrays to support vs not.