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

Jump to bottom

Indentation within generate construct after always block is wrong if generate/endgenerate omitted #1257

Closed
veripoolbot opened this issue Jan 2, 2018 · 7 comments
Closed

Indentation within generate construct after always block is wrong if generate/endgenerate omitted #1257

veripoolbot opened this issue Jan 2, 2018 · 7 comments
Labels
indents

Comments

@veripoolbot
Copy link
Collaborator

Author Name: Dan Hopper
Original Redmine Issue: 1257 from https://www.veripool.org

There's an interesting difference in indentation in the below example module. The assign statement after an always block within a generate construct will be correctly indented if it's within a generate region (explicit generate/endgenerate lines), and incorrectly indented if the optional generate region (generate/endgenerate) is omitted.

If you load the below snippet in and reformat, the commented assign statement will be (incorrectly) indented to column 0. If you remove the comments on the generate/endgenerate pairs, it will indent properly. According to SV 1800-2012, the generate/endgenerate pair is optional, so ideally it would treat both versions identically.

I'm not proficient in Lisp so it wasn't immediately obvious to me how to fix the .el file to eliminate.

This occurs with verilog-mode-2017-12-21-39c77b2-vpo.el and also at least as far back as verilog-mode-842.el.

Thanks,
Dan

module t1
 (
 );

genvar pipe;
logic [1:0] v;
logic x;

//generate
for (pipe=0; pipe<2; pipe++) begin : v_bl
     always_comb begin
         assign v[pipe] = '0;
     end
     assign x = '0;  // will be incorrectly indented to column 0
end
//endgenerate

endmodule
@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-01-03T13:50:18Z

Seems so. The indentation code is generally maintained by patches, so if you could contribute a fix that would be great, otherwise it might be a while before it is fixed.

@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: David Rogoff
Original Date: 2019-05-10T01:04:00Z

I just hit this. What's really weird is if I have the same generate loop twice but have another one in the middle with generate/endgenerate, the first loop will be messed up but the 2nd, identical copy will be indented correctly!

So, still no fix after a year? Wish my elisp skill was anywhere near good enough to help fix this.

David

module indent;

    // CASE 1 indented wrong.  See end of module
    for (genvar aa = 0; aa < FF; aa++) begin : gen_hh
       for (genvar bb = 0; bb < FF; bb++) begin : gen_ii
          if (`asdf [aa]) begin : gen_jj

             always_ff @ (negedge cc [aa][bb]) begin
                if (dd [aa][bb])) begin
                   for (uint_t d_idx = 0; d_idx < 16; d_idx++)
                     ee [aa][bb].push_front (tx_dfe_out [aa][bb]  [15 - d_idx]);
                end
             end

    always_ff @ (posedge hs_clk) begin
       ee_size [aa][bb] = ee [aa][bb].size();
       if (ee_size [aa][bb] > 0)
         gg [aa][bb] <= ee [aa][bb].pop_front;
    end

end : gen_jj
       end : gen_ii
    end : gen_hh



    // this indents correctly without generate/endgenerate
    for (genvar aa = 0; aa < FF; aa++) begin : gen_hh
       for (genvar bb = 0; bb < FF; bb++) begin : gen_ii
          if (`asdf [aa]) begin : gen_jj
             assign a[aa][bb] = aa + bb;
             assign b[aa][bb] = aa + bb;
          end : gen_jj
       end : gen_ii
    end : gen_hh


    // this works now with gen/endgen
    generate
       for (genvar aa = 0; aa < FF; aa++) begin : gen_hh
          for (genvar bb = 0; bb < FF; bb++) begin : gen_ii
             always @ (negedge cc)
               a = 5;
             always @ (negedge dd)
               w = 2;
          end : gen_ii
       end : gen_hh
    endgenerate


    // CASE 1 again. No change but works - apparently since verilog-mode hit a generate statement above this line
    for (genvar aa = 0; aa < FF; aa++) begin : gen_hh
       for (genvar bb = 0; bb < FF; bb++) begin : gen_ii
          if (`asdf [aa]) begin : gen_jj

             always_ff @ (negedge cc [aa][bb]) begin
                if (dd [aa][bb])) begin
                   for (uint_t d_idx = 0; d_idx < 16; d_idx++)
                     ee [aa][bb].push_front (tx_dfe_out [aa][bb]  [15 - d_idx]);
                end
             end

             always_ff @ (posedge hs_clk) begin
                ee_size [aa][bb] = ee [aa][bb].size();
                if (ee_size [aa][bb] > 0)
                  gg [aa][bb] <= ee [aa][bb].pop_front;
             end

          end : gen_jj
       end : gen_ii
    end : gen_hh

endmodule : indent

@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: David Rogoff
Original Date: 2019-05-10T01:06:21Z

Even stranger - If I just put

    generate
    endgenerate

near the top of my module, everything after is indented correctly.

Good work-around for now and maybe good clue for finding the verilog-mode problem!

David

@punzik
Copy link
Contributor

punzik commented Sep 14, 2021 •
edited

This bug can be fixed as shown below, but I am not sure if this is correct from an architectural point of view.

diff --git a/verilog-mode.el b/verilog-mode.el
index 8370d4a..248c1f5 100644
--- a/verilog-mode.el
+++ b/verilog-mode.el
@@ -4759,7 +4759,7 @@ More specifically, after a generate and before an endgenerate."
 	(while (and
 		(/= nest 0)
 		(verilog-re-search-backward
-                "\\<\\(module\\)\\|\\(connectmodule\\)\\|\\(generate\\)\\|\\(endgenerate\\)\\>" nil 'move)
+                "\\<\\(module\\)\\|\\(connectmodule\\)\\|\\(generate\\)\\|\\(endgenerate\\)\\|\\(if\\)\\>" nil 'move)
 		(cond
 		 ((match-end 1) ; module - we have crawled out
 		  (throw 'done 1))
@@ -4768,7 +4768,9 @@ More specifically, after a generate and before an endgenerate."
                 ((match-end 3) ; generate
 		  (setq nest (1- nest)))
                 ((match-end 4) ; endgenerate
-		  (setq nest (1+ nest))))))))
+		  (setq nest (1+ nest)))
+                ((match-end 5) ; if
+		  (setq nest (1- nest))))))))
     (= nest 0) )) ; return nest

 (defun verilog-in-fork-region-p ()

@wsnyder
Copy link
Member

wsnyder commented Sep 15, 2021

I thought that might mess up some normal "if" indentation, but it seems OK.

If [generate] "if" is there, then probably [generate] "case" also needs to be?

Could you make a pull request including this and test cases for the if/case?

@punzik
Copy link
Contributor

punzik commented Sep 15, 2021

Could you make a pull request including this and test cases for the if/case?

Yes, sure.

@gmlarumbe
Copy link
Contributor

Seems to be fixed in 9def905

@dbhopper dbhopper mentioned this issue Jan 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
indents
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@wsnyder @punzik @gmlarumbe @veripoolbot