bmv2 "array index must be a constant" error

[hesingh]

I am using a header stack in v1model p4-16 code. The index into the header stack array is obtained from a header in the packet and index copied to metadata. Obviously, the header is parsed in the parser. p4c-bm2-ss fails because the index is not a compile-time constant. So, how is such code supposed to work? The p4test backend does not care and compiles the code fine.

./testdata/p4_16_samples/ipv6-switch-ml-bmv2.p4(111): [--Werror=invalid] error: meta._ml_idx4: Invalid all array indexes must be constant
             hdr.vector[0].e = pool[meta.ml_idx].val;
                                    ^^^^

[jafingerhut]

Section 8.15 "Operations on header stacks" of the P4_16 language spec: https://p4.org/p4-spec/docs/P4-16-v1.1.0-spec.html#sec-expr-hs

says this: "Some architectures may impose the constraint that the index expression evaluates to a compile-time known value"

v1model is such an architecture that imposes this constraint.

If you want to access one of a handful of different header stack elements in actions depending upon a run-time varying value, one workaround for the v1model restriction is to include the run-time variable index as a table search key, which selects between several different actions that are identical, except for the constant array index values they use.

[hesingh]

Right - I do understand why bmv2 imposes the restriction because there is no JSON one can generate for a non-compile-time index. Sorry, I forgot about using tables - make sense, thanks!

[hesingh]

It's not a handful of elements. I have 128 and each can be used a index for the header stack array.

[hesingh]

Both the issues I filed today are related to switch Machine Learning (ML) where array elements are added and array indices are used for aggregating ML data into different aggregators. I think the switch ML on Tofino may be using p4-14. I am writing p4-16 and v1model for the IETF hackathon this weekend.

[jafingerhut]

The approach of multiple actions works for an arbitrary finite number of elements. I only mentioned a handful since I was guessing that might be the case, but apparently it is not for what you want. Again, auto-generation/printing of repetitive code might be your friend here.

I agree that it would be nice if all P4 targets supported run-time variable indexes of arrays. The fact is that the statement is in the language spec precisely because someone wanted to caution people about expecting that, at least at the time it was written, and I doubt that the highest speed implementations have changed significantly since that writing.

[hesingh]

Agree on auto-generation - thanks. Maybe, we keep this issue open and explore what's it take for the compiler to support run-time array index. Also, in April 2019, using p4c of that time, I could use if-statement inside action. Now, I can't.

[jafingerhut]

Some if statements inside of actions should still work just fine. If you have an example of one that used to work with older p4c and doesn't with latest p4c, I'd be interested in seeing an example.

It is not all if statements that used to work -- if the compiler can figure out how to auto-translate them into using a ternary operator on the right-hand side of an assignment statement, it should work, e.g. (expression) ? (true_expr) : (false_expr). If the compiler cannot figure out how to auto-translate it into that form, I do not believe p4c has ever supported it.

See here for some details: https://github.com/p4lang/behavioral-model/blob/master/docs/simple_switch.md#restrictions-on-code-within-actions

[hesingh]

This is what worked inside an action during April 2019, but did not later. Don't have exact dates handy.

        if (hdr.ipv6.isValid() || hdr.inner_ipv6.isValid()) {
        hdr.vlan_tag_[0].etherType = 16w0x86dd;
    } else {
        hdr.vlan_tag_[0].etherType = 16w0x800;
    }

[jafingerhut]

I just tried making that the body of an action in a program where those headers were defined, and it compiled fine with the latest p4c --target bmv2 --arch v1model myprog.p4 command, and write a BMv2 JSON file.

Are you getting a compilation error when trying to compile a program with such an if statement inside of an action? Or it produces a BMv2 JSON file that doesn't seem to be working correctly? Something else?

[hesingh]

I too tried ./p4c-bm2-ss --std p4-16 foo.p4 with the if-statements above in action body and p4c behaves fine.

Now I remember what transpired. When I moved my code to run on another target hardware, the action body with if-statement was not accepted.

[jnfoster]

This is not an appropriate forum for discussing the capabilities or restrictions of backends for proprietary P4_16 targets.

[hesingh]

@jnfoster Sorry, my bad. I edited the comment.

[hesingh]

@jafingerhut I wonder if the v1model.p4 could be changed to allow run-time indices. If not, would the PSA model consider making the change to the bmv2 backend?

[mihaibudiu]

How would you change v1model?

[hesingh]

Sorry, I used incorrect terminology.

It's the v1model.p4 architecture being used by the bmv2 simple_switch backend that imposes the constraint. The backend would need a change.

[jafingerhut]

It seems that modifying the open source tools p4c and behavioral-model to support this would be some changes to each of them. e.g. I think this might cover it, but haven't gone very far in examining the details:

• add a way to the BMv2 JSON data file to provide a run-time expression to use as the index, when accessing a header stack
• generate that new BMv2 JSON construct from the p4c BMv2 back end
• verify that the p4c front end can handle run-time variables as header stack index expressions, without violating any constraints or assumptions made when its internals were developed, enhancing any parts where that assumption was formerly baked into the code.

All of that could be done for v1model and/or PSA.

I suspect that more accurate than the current statement in the spec "Some architectures may impose the constraint that the index expression evaluates to a compile-time known value" is this: "Some targets may ...". Making all of the changes above would encourage target developers to consider supporting run-time index values, but it would not magically change a target that physically could not do it, restricted by its design, into one that could.

[hesingh]

I always start with p4c frontend to see if a new feature requires any change. The Stack's next, last, push, pop can only be used in the P4 parser. The ML paper has algorithm 1 on page 4 using run-time index.

https://arxiv.org/pdf/1903.06701.pdf

We may have to use the run-time index outside of the P4 parser in an ingress control. Also, I think, the Stack IR may change to incorporate a run-time index.

Thereafter, the next crucial issue is to define JSON for the new run-time index.

Another operative question is for asic/FPGA vendors if they can support such a feature in their hardware. Intel asic and FPGA/Mellanox/Cisco/Xilinx should weigh in. If that's not possible without signing NDA, never mind.

[jafingerhut]

Minor correction to your previous comment: The P4_16 language spec restricts next and last on header stacks to within parsers.

It makes no such restrictions on push_front and pop_front operations, and the current p4c and behavioral-model implementations allow those operations to be invoked during control execution. In fact, for those, it is not obvious to me whether they work if invoked inside of a parser -- they might, but I doubt there is much reason a P4 program author would want to perform those operations in a parser, so it seems fairly low priority to find out what the current implementation does there.

In general, anything that can be done in software could be done in an FPGA, without changing the FPGA hardware design.

ASICs are the targets that might have designs preventing them from doing this. If you expect a public answer to this Github comment on this topic, I expect you will be disappointed (but I am happy to be proved wrong on that).

[hesingh]

See this line of code in that restricts push and pop of Stack to be allowed only in parser.

https://github.com/p4lang/p4c/blob/master/frontends/p4/typeChecking/typeChecker.cpp#L2642

Regarding asics, even I was pessimistic about supporting a run-time index. If asics can't support this feature, why bother changing p4c and bmv2 backend? After all switch ML has to run on an asic.

[hesingh]

What I call as "Stack" is Type_Stack in p4c code. Type_Stack represents a header stack.

[antoninbas]

A correction to Andy's previous comment: bmv2 already supports dynamic indices for header stacks. This is done using the dereference_header_stack binary operand in the bmv2 JSON (see the doc), which takes as parameters the name of the header stack and an expression evaluating to a valid index for that header tack.

[jafingerhut]

@hesingh I do not know what the code in the typeChecker.cpp source file that you link to actually does. I do know that I wrote, checked in, and every commit to p4c code tests, the test P4_16 program linked below, which has an STF test to send packets through bmv2 simple_switch, that uses push_front and pop_front operations on header stacks, invoked from a control, not a parser.

https://github.com/p4lang/p4c/blob/master/testdata/p4_16_samples/header-stack-ops-bmv2.p4 https://github.com/p4lang/p4c/blob/master/testdata/p4_16_samples/header-stack-ops-bmv2.stf

[jafingerhut]

Thanks for the correction, Antonin. Either I never saw that before, or had forgotten about it.

[jafingerhut]

Regarding this comment: "Regarding asics, even I was pessimistic about supporting a run-time index. If asics can't support this feature, why bother changing p4c and bmv2 backend? After all switch ML has to run on an asic."

I was not saying that I know ASICs cannot do it. I was trying to say that getting public answers about existing ASICs that they can/cannot do it seems unlikely. Not impossible, just unlikely.

Also, a potential reason to implement it in the open source tools is that those targets that can do it, can take advantage of the implementation in p4c. If you take the long view, it might encourage future ASICs to support it that might not otherwise do so.

[hesingh]

@hesingh I do not know what the code in the typeChecker.cpp source file that you link to actually does. I do know that I wrote, checked in, and every commit to p4c code tests, the test P4_16 program linked below, which has an STF test to send packets through bmv2 simple_switch, that uses push_front and pop_front operations on header stacks, invoked from a control, not a parser.

https://github.com/p4lang/p4c/blob/master/testdata/p4_16_samples/header-stack-ops-bmv2.p4 https://github.com/p4lang/p4c/blob/master/testdata/p4_16_samples/header-stack-ops-bmv2.stf

Sorry, my bad. This is the code I pointed to:

if (parser != nullptr)
                typeError("%1%: '%2%' and '%3%' for stacks cannot be used in a parser",
                          expression, IR::Type_Stack::push_front, IR::Type_Stack::pop_front);

push and pop are NOT allowed in parser. So, indeed, you are correct push and pop are allowed inside a control.

[hesingh]

Regarding this comment: "Regarding asics, even I was pessimistic about supporting a run-time index. If asics can't support this feature, why bother changing p4c and bmv2 backend? After all switch ML has to run on an asic."

I was not saying that I know ASICs cannot do it. I was trying to say that getting public answers about existing ASICs that they can/cannot do it seems unlikely. Not impossible, just unlikely.

Also, a potential reason to implement it in the open source tools is that those targets that can do it, can take advantage of the implementation in p4c. If you take the long view, it might encourage future ASICs to support it that might not otherwise do so.

Ok, then, let's investigate the feature for a p4c and bmv2 implementation - thanks. Thanks also to @antoninbas for the reference to bmv2 code.

[hesingh]

A correction to Andy's previous comment: bmv2 already supports dynamic indices for header stacks. This is done using the dereference_header_stack binary operand in the bmv2 JSON (see the doc), which takes as parameters the name of the header stack and an expression evaluating to a valid index for that header stack.

See this error from p4c saying error: hdr.ml.idx: Invalid all array indexes must be constant hdr.vector[0].e = pool[hdr.ml.idx].val;. I am using

The P4 code can be found at: https://github.com/IETF-Hackathon/p4-ipv6-switch-ml

p4c-bm2-ss --std p4-16 ~/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4 
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(106): [--Wwarn=uninitialized_use] warning: [].val may be uninitialized
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
                                ^^^^^^^^^^^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(107): [--Wwarn=uninitialized_use] warning: [].cnt may be uninitialized
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
                                 ^^^^^^^^^^^^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(109): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
             hdr.vector[0].e = pool[hdr.ml.idx].val;
                                    ^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(110): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
             pool[hdr.ml.idx] = {0};
                  ^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(111): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
             count[hdr.ml.idx] = {0};
                   ^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(106): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
              ^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(106): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
                                     ^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(107): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
               ^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(107): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
                                       ^^^^^^^^^^
hemant@ubuntu:~/p4c/build$ 

[jafingerhut]

@hesingh Antonin's comment was that bmv2 simple_switch has a way to do this, if p4c could only generate the correct BMv2 JSON file.

Your comment shows that p4c does not generate the correct BMv2 JSON file. No argument there.

[hesingh]

@hesingh Antonin's comment was that bmv2 simple_switch has a way to do this, if p4c could only generate the correct BMv2 JSON file.

Your comment shows that p4c does not generate the correct BMv2 JSON file. No argument there.

I see, thanks, @jafingerhut. Then, let's see if p4c can fix the issue.

[hesingh]

Do note, simple_switch injests JSON output from p4c. This is the reason it makes sense to fix p4c-bm2-ss for this issue.

[hesingh]

WIth latest p4c from today, I see the left-hand side of a statement causing an error which was not seen by p4c few days back. Few days back, the right-hand side of a statement caused error with header stack index not being a constant.

hemant@ubuntu:~/p4c/build$ ./p4c-bm2-ss --std p4-16 ~/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4 
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(106): [--Wwarn=uninitialized_use] warning: [].val may be uninitialized
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
                                ^^^^^^^^^^^^^^^^^^^^
/home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(107): [--Wwarn=uninitialized_use] warning: [].cnt may be uninitialized
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
                                 ^^^^^^^^^^^^^^^^^^^^^
In file: /home/hemant/p4c/backends/bmv2/common/expression.cpp:97
Compiler Bug: /home/hemant/p4-ipv6-switch-ml/ipv6-switch-ml-bmv2.p4(109): hdr.vector[0];: could not convert to Json
             hdr.vector[0].e = pool[hdr.ml.idx].val;
             ^^^^^^^^^^^^^

hemant@ubuntu:~/p4c/build$ 

The error is from this line of code in p4c.

https://github.com/p4lang/p4c/blob/master/backends/bmv2/common/expression.cpp#L97

[hesingh]

The P4 code used can be found at https://github.com/IETF-Hackathon/p4-ipv6-switch-ml

[jafingerhut]

I know the change in behavior you are seeing is related to this issue, but it might be easier to focus on the change in behavior if you create a separate issue from this one, since the fix for each is likely different.

[mihaibudiu]

I cannot reproduce this issue with the master branch of p4c.

[ChrisDodd]

When I compile the linked code with latest p4c-bmv2-ss, I just get the "array indexes must be constant" errors:

ipv6-switch-ml-bmv2.p4(106): [--Wwarn=uninitialized_use] warning: [].val may be uninitialized
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
                                ^^^^^^^^^^^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(107): [--Wwarn=uninitialized_use] warning: [].cnt may be uninitialized
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
                                 ^^^^^^^^^^^^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(109): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
             hdr.vector[0].e = pool[hdr.ml.idx].val;
                                    ^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(110): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
             pool[hdr.ml.idx] = {0};
                  ^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(111): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
             count[hdr.ml.idx] = {0};
                   ^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(106): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
              ^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(106): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;
                                     ^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(107): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
               ^^^^^^^^^^
ipv6-switch-ml-bmv2.p4(107): [--Werror=invalid] error: hdr.ml.idx: Invalid all array indexes must be constant
         count[hdr.ml.idx].cnt = count[hdr.ml.idx].cnt + 1;
                                       ^^^^^^^^^^

[hesingh]

Sorry, I had changed p4c/backends/bm2/common.cpp in my build and that caused the different error. Once I backed out the change, p4c is just throws the "Invalid all array indexes must be constant" error.

[hesingh]

In the behavioral-model, which supports header stack with array index at run-time, the following op is defined in code:

https://github.com/p4lang/behavioral-model/blob/7192f7550b20859e9f5f1fb8759f2d07e912b6ae/include/bm/bm_sim/expressions.h#L52

For p4c to support dereference_header_stack, should we define a new Operation_deref_hdr_stack in p4c/ir/expression.def?

[ChrisDodd]

Why define a new operation, instead of just using the existing IR::ArrayIndex? What is the gain? While you could do so, it seems that the effect is the same as ArrrayIndex, so you'd need to duplicate all code for handling ArrayIndex to also deal with the new node (at least if you need to run the pass in question after introducing the new node type)

[ChrisDodd]

IMO the obvious way of handling this is in ExpressionConverter::postorder(const IR::ArrayIndex* expression), if the right operand is not a constant, you create this DEREFERENCE_HEADER_STACK json node instead of giving an error.

[hesingh]

Thanks, @ChrisDodd

[hesingh]

@antoninbas or @ChrisDodd For this P4 statement:

pool[hdr.ml.idx].val = pool[hdr.ml.idx].val + hdr.vector[0].e;

should the ArrayIndex in JSON for right-hand side be

"value" : ["pool_0[hdr.ml.idx]", "val"]

or

"value" : ["pool_0[DEREFERENCE_STACK]", "val"]

or anything else so that the JSON can interoperate with the behavioral-model.

[hesingh]

I do not see any P4 code in the behavioral-model which is using non-constant array index. This is the issue with trying to figure out what to write for JSON object for dereference_stack in p4c. Looking at the behavioral-model test code, I see DEREFERENCE_HEADER_STACK being used below.

https://github.com/p4lang/behavioral-model/blob/fff3171f2a7e3fcbb2ae8dec9aa425e282025b10/tests/test_conditionals.cpp#L680

However, I am not sure what name and index are.

This is what I have changed for the bmv2 backend so far merely to test but it's not final code.

diff --git a/backends/bmv2/common/expression.cpp b/backends/bmv2/common/expression.cpp
index 5246aeef..48f2a558 100644
--- a/backends/bmv2/common/expression.cpp
+++ b/backends/bmv2/common/expression.cpp
@@ -204,7 +204,9 @@ void ExpressionConverter::postorder(const IR::ArrayIndex* expression)  {
     }

     if (!expression->right->is<IR::Constant>()) {
-        ::error(ErrorType::ERR_INVALID, "all array indexes must be constant", expression->right);
+        // create a DEREFERENCE_HEADER_STACK json node
+        auto str = expression->right->toString();
+        elementAccess += "[" + str + "]";
     } else {
         int index = expression->right->to<IR::Constant>()->asInt();
         elementAccess += "[" + Util::toString(index) + "]";

[ChrisDodd]

I would imagine it would be more like:

  "value" : [ {
    "type" : "expression",
    "value" : {
      "op" : "DEREFERENCE_STACK",
      "left" : { "type" : "stack", "value" : "pool_0" },
      "right" : { "type" : "field", "value" : [ "hdr", "ml", "idx" ] } } },
    "val" ]

but I'm not familiar with the exact syntax supported by bmv2 in these json expressions.

[antoninbas]

I think the documentation is quite clear about how it works. It is not different from other binary operands. And the proof is that @ChrisDodd was able to figure it out pretty fast without being familiar with the JSON format :)

[hesingh]

I changed the p4c bmv2 backend. This is the line of P4 code I am generating JSON for:

hdr.vector[0].e = pool[hdr.ml.idx].val;

But the JSON causes Compiler Bug: 0x55b016900f60: Unexpected json/. The error is not too helpful to a new user working with p4c's JSON.

{
  "value" : {
    "value" : {
      "op" : "DEREFERENCE_STACK",
      "left" : {
        "type" : "stack",
        "value" : "pool_0"
      },
      "right" : {
        "type" : "field",
        "value" : ["hdr", "ml", "idx"]
      }
    },
    "type" : "expression"
  }
}
In file: /home/hemant/p4c/backends/bmv2/common/expression.cpp:447
Compiler Bug: 0x55c254de2cc0: Unexpected json

[jafingerhut]

I am pretty sure that modifying p4c is not a "new user experience" kind of thing. Power users only, who can do lots of debugging on their own :-)

That said, I do not know whether anyone is willing to look at and help debug your changes, but if you publish them, it is possible, whereas it is impossible if you do not.

[hesingh]

Actually, writing the p4c code to generate new JSON is pretty trivial for me.

I am saying I did what the JSON was generally asked to be generated but there is still something missing with JSON.

[jafingerhut]

The error message is coming from p4c code, yes? Not from simple_switch?

[hesingh]

The error message is coming from p4c code, yes? Not from simple_switch?

Yes, it's from p4c because I am developing p4c code to support JSON output for run-time array index.

[hesingh]

I am getting there. In the p4c bmv2 backend code the "Unexpected json" error is from an "else" block because now the ArrayIndex is now a JSON object. I added code to deal with this case and write more code.