Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dslx_fmt should not delete code w/ waiver #1746

Closed
proppy opened this issue Nov 28, 2024 · 4 comments
Closed

dslx_fmt should not delete code w/ waiver #1746

proppy opened this issue Nov 28, 2024 · 4 comments
Labels
bug Something isn't working or is incorrect dslx:fmt DSLX auto-formatter

Comments

@proppy
Copy link
Member

proppy commented Nov 28, 2024

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.

@proppy proppy added dslx:fmt DSLX auto-formatter bug Something isn't working or is incorrect labels Nov 28, 2024
@dplassgit dplassgit moved this to Needs investigation in XLS usability Dec 3, 2024
@dplassgit
Copy link
Collaborator

Might be a duplicate of #1735

@dplassgit
Copy link
Collaborator

I think this happens because the AST contains two copies of everything from a proc: the "extracted" methods (e.g., matmul.init, matmul.config, matmul.next) and the "original" proc node. They're processed in sequence, and the format disabler is getting confused regarding which nodes should be converted to VerbatimNodes.

@dplassgit
Copy link
Collaborator

#1029 describes the issue more.

@dplassgit
Copy link
Collaborator

Duplicate of #1735

@dplassgit dplassgit marked this as a duplicate of #1735 Dec 4, 2024
@dplassgit dplassgit closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in XLS usability Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working or is incorrect dslx:fmt DSLX auto-formatter
Projects
Status: Done
Development

No branches or pull requests

2 participants