Evaluator regression

[quark17]

There appears to have been a regression in October 2020, in revision bad371b3, where @nanavati changed improveIf to not optimize constructors with undefined. On the b-lang-discuss mailing list, an example was attached to message 712 (here), which I have boiled down to a small example:

import Vector::*;

`define Q_SIZE 8
typedef Bit#(2) T;

(*synthesize*)
module mkSample_code();

  Vector#(`Q_SIZE, Reg#(T))           vec_data  <- replicateM(mkRegU);
  Reg #(Maybe#(Vector#(`Q_SIZE, T)))  rg_1   <- mkRegU;
  Reg #(Maybe#(Vector#(`Q_SIZE, T)))  rg_2   <- mkRegU;
  Reg #(Vector#(`Q_SIZE, Bool))       rg_3   <- mkRegU;
  Reg #(Maybe#(Vector#(`Q_SIZE, T)))  rg_4   <- mkRegU;
  Reg #(Vector#(`Q_SIZE, Bool))       rg_5   <- mkRegU;

  rule r;
    Vector#(`Q_SIZE, T) tmp_data = readVReg (vec_data);

    if (rg_1 matches tagged Valid .d1)
      tmp_data  = d1;

    if (rg_2 matches tagged Valid .d2)
      begin
        for (int i = 0 ; i< `Q_SIZE ;i = i+1)
          begin
            if (rg_3[i] == True)
              tmp_data[i] = d2[i];
            end
      end

    if (rg_4 matches tagged Valid .d4)
      begin
        for (int i = 0 ; i< `Q_SIZE ;i = i+1)
          begin
            if (rg_5[i] == True)
              tmp_data[i] = d4[i];
          end
      end

    writeVReg(vec_data,tmp_data);
  endrule

endmodule

With older BSC (as far back as 2018 at least), it compiles in under a second; with newer BSC (at commit bad371b3 through to the latest release), it fails due to exceeding the max number unfolding steps.

The code is assigning a temporary variable with the values from a vector of registers, then dynamically adjusting elements of the variable's value, and then writing the variable's final values back to the registers. The adjustments are the following:

1. If rg_1 is valid, then replace the temp with that valid value
2. If rg_2 is valid, then update elements from that valid value, when the corresponding value in rg_3 is true (thus using rg_3 as enable signals for the the data in rg_2)
3. The same as step 2, but using rg_4 and rg_5 as the data and enable signals

This should simplify. I have not investigated why the commit broke things or what alternate optimization might help. Ravi, is that something you could look at?

What is interesting is that removing step 1 causes the code to compile again. Steps 2 and 3 are for-loops with vector indexing and updates; but step 1 is just a single if-statement (and no vector indexing).

If the change to improveIf is not vital to how BSC evaluates, perhaps a command-line flag can be added, to select the old behavior when people need it?

A workaround is to re-write the code to manually push the logic into the elements of the vector:

  rule r;
    Vector#(`Q_SIZE, T) tmp_data = readVReg (vec_data);

    for (int i = 0 ; i< `Q_SIZE ;i = i+1)
      begin
        if (rg_1 matches tagged Valid .d1)
          tmp_data[i]  = d1[i];

        if (rg_2 matches tagged Valid .d2)
          if (rg_3[i] == True)
            tmp_data[i] = d2[i];

        if (rg_4 matches tagged Valid .d4)
          if (rg_5[i] == True)
            tmp_data[i] = d4[i];
      end

    writeVReg(vec_data,tmp_data);
  endrule

[quark17]

The change in BSC was to stop merging constructors with don't-care values, but this example doesn't have a don't-care. But maybe the BSV parser is introducing something, and maybe this can be fixed in the parser (or the optimization adjusted to recognize specifics of the parser's construction).

BSV code that conditionally updates a variable, like this:

if (rg_1 matches tagged Valid .d1)
  tmp_data  = d1;

is parsed into internal code that looks like this:

letseq  _theResult__ :: Vector 8 T
        _theResult__ = 
          case () of {
            _ when Valid d1 <- rg_1 -> ...
            _ -> tmp_data

        tmp_data = _theResult_

Maybe those don't-cares in the case-arm patterns are relevant?