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

Formatting with verible-verilog-format 2020-09-22 #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hzeller
Copy link
Owner

@hzeller hzeller commented Sep 23, 2020

version:
v0.0-620-gb46d580
Commit 2020-09-22
Built 2020-09-22T16:35:05Z

command:
verible-verilog-format --formal_parameters_indentation=indent --named_parameter_indentation=indent --named_port_indentation=indent --port_declarations_indentation=indent --inplace ${filename}

version:
v0.0-620-gb46d580
Commit  2020-09-22
Built   2020-09-22T16:35:05Z

command:
verible-verilog-format --formal_parameters_indentation=indent --named_parameter_indentation=indent --named_port_indentation=indent --port_declarations_indentation=indent --inplace ${filename}
Copy link

@fangism fangism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 40% through file list...

parameter int unsigned PMPNumRegions = 4,
parameter bit RV32E = 1'b0,
parameter ibex_pkg::rv32m_e RV32M = ibex_pkg::RV32MFast
parameter bit DbgTriggerEn = 1'b0,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, 2 vs. 4 spaces again. The original style 4 spaces is what our formatter does by default, but OpenTitan wanted 2 spaces indented for module formal parameters.

inout wire clk_i,
inout wire in_rst_ni,
output wire test_passed_o
// Clock and Reset
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, module ports 2 vs. 4 spaces.

@@ -15,20 +15,20 @@ module ibex_riscv_compliance (
input IO_RST_N
);

parameter bit PMPEnable = 1'b0;
parameter bit PMPEnable = 1'b0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want local parameter declarations (as module items) to be aligned, need new issue.

.host_rvalid_o(host_rvalid),
.host_rdata_o (host_rdata),
.host_err_o (host_err),

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: one blank line is separating alignment groups here, but authors might have wanted one alignment group.

input wdata;
output rdata;
output error;
input reset;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs new issue if you want these aligned.

`endif
`ifndef RVFI
$fatal(
"Fatal error: RVFI needs to be defined globally."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another overly aggressive line breaking.

logic [ 6:0] dummy_set;
logic [ 2:0] dummy_opcode;
logic [ 31:0] dummy_instr;
logic [31:0] dummy_instr_seed_q, dummy_instr_seed_d;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was ignored for alignment purposes because it contains a multi-declaration. It could be handled potentially.

@@ -82,14 +82,14 @@ module ibex_ex_block #(
// Intermediate Value Register Mux
assign imd_val_d_o[0] = multdiv_sel ? multdiv_imd_val_d[0] : {2'b0, alu_imd_val_d[0]};
assign imd_val_d_o[1] = multdiv_sel ? multdiv_imd_val_d[1] : {2'b0, alu_imd_val_d[1]};
assign imd_val_we_o = multdiv_sel ? multdiv_imd_val_we : alu_imd_val_we;
assign imd_val_we_o = multdiv_sel ? multdiv_imd_val_we : alu_imd_val_we;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wasn't this continuous assignment statement aligned with the ones above it? Needs new bug.

assign rdata_d[i] = valid_q[i+1] ? rdata_q[i+1] : in_rdata_i;
assign err_d [i] = valid_q[i+1] ? err_q [i+1] : in_err_i;
assign rdata_d[i] = valid_q[i+1] ? rdata_q[i+1] : in_rdata_i;
assign err_d[i] = valid_q[i+1] ? err_q[i+1] : in_err_i;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why were these not aligned?

logic output_compressed;
logic skid_valid_d, skid_valid_q, skid_en;
logic [15:0] skid_data_d, skid_data_q;
logic skid_err_q;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are only locally aligned because they are sandwiched between multi-variable declarations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants