google / xls

XLS: Accelerated HW Synthesis
http://google.github.io/xls/
Apache License 2.0
1.22k stars 181 forks source link

dslx_fmt should not delete code w/ waiver #1746

Open proppy opened 1 day ago

proppy commented 1 day ago

Describe the bug It seems that the formatter sometimes erroneously delete code when dslx-fmt:: waiver are being used.

To Reproduce

Running dslx_fmt on:

proc matmul<ROWS: u32, COLS: u32> {
    south_outputs: chan<F32>[COLS][ROWS] out;
    west_inputs: chan<F32>[COLS][ROWS] in;

    config(activations_in: chan<F32>[ROWS] in, results_out: chan<F32>[COLS] out) {
        // Declare the east-to-west channels.
        let (east_outputs, west_inputs) = chan<F32>[COLS][ROWS]("east_west");

        // Declare the north-to-south channels.
        let (south_outputs, north_inputs) = chan<F32>[COLS][ROWS]("north_south");

        // Spawn all the procs. Specify weights to give a "mul-by-two" matrix.
        let f32_0 = float32::zero(false);
        let f32_2 = F32 { sign: false, bexp: u8:128, fraction: u23:0 };

        let weights = F32[COLS][ROWS]:[
          // dslx-fmt::off
          [2, 0, 0, 0],
          [0, 2, 0, 0],
          [0, 0, 2, 0],
          [0, 0, 0, 2]
          // dslx-fmt::on
        ];
        (south_outputs, west_inputs)
    }

    init { () }

    // All we need to do is to push in "zero" values to the top of the array and consume void
    // channels to keep the system moving.
    next(state: ()) {
      ()
    }
}

"reformat" the code into:

// Declare the east-to-west channels.

// Declare the north-to-south channels.

// Spawn all the procs. Specify weights to give a "mul-by-two" matrix.

// dslx-fmt::off
          [2, 0, 0, 0],
          [0, 2, 0, 0],
          [0, 0, 2, 0],
          [0, 0, 0, 2]
          // dslx-fmt::on
        ];
        (south_outputs, west_inputs)
    }

    init { () }

    // All we need to do is to push in "zero" values to the top of the array and consume void
    // channels to keep the system moving.
    next(state: ()) {
      ()
    }
}

Expected behavior The proc definition above should not be deleted.

Additional context

Note that removing the dslx-fmt:: waiver workaround this issue and properly reformat the code.