Formatter: Avoid whitespace in bit selection

[GregAC]

The formatter introduces whitespace around operators inside bit-selections. I'd argue these are better off as compact expressions, I note whitespace isn't added in declarations.

For example in https://github.com/lowRISC/ibex/blob/master/rtl/ibex_icache.sv

Before formatting:

logic [INDEX_W-1:0]                  inval_index_d, inval_index_q;

After formatting:

logic [INDEX_W-1:0] inval_index_d, inval_index_q;

The extra whitespace between the dimension and the names is removed but the expression inside the [] remains compact.

Before formatting:

assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W-1:LINE_W],{LINE_W{1'b0}}};

After formatting:

assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W - 1:LINE_W], {LINE_W{1'b0}}};

Similar syntax (though used to select bits rather than declare dimensions) but this time whitespace is added around the -

[fangism]

Confirming the behavior you observe.

Reference: https://github.com/google/verible/blob/19dade32b36684002ea4f88a9b8786d5d3402b92/verilog/formatting/token_annotator.cc#L389

https://github.com/google/verible/blob/19dade32b36684002ea4f88a9b8786d5d3402b92/verilog/formatting/token_annotator.cc#L294

What is happening here is that we've decided for the time being to limit spacing around : to 1 space maximum in declarative range contexts like ports, and symmetrize the right spacing based on the original spacing to the left of the :. The style guide we've been implementing so far is a parent of the lowRISC guide, and we only decided to allow compaction in some context, but decided not to force 0-spacing in the formatter tool at this time -- in other words, we've partially punted with the current half-solution.

https://github.com/google/verible/blob/19dade32b36684002ea4f88a9b8786d5d3402b92/verilog/formatting/token_annotator.cc#L689

The Preserve here means, "keep the original spacing", which explains why some spaces are untouched.

Indexing/bit-slicing have not been singled out as special cases at this time, so by default the spacing around binary operators is 1 (see near the above code excerpts). But this would be easy to do because the formatter knows the full syntactic context of every token.

[ghost]

Hi @fangism I've created PR that forces compaction of expressions in bit selection ranges: #558 I'm not sure what to do with ranges in declaration contexts (leaved as it is right now).

Also I've noticed that master branch is correctly preserving spaces in example expressions - only limiting to 1 space and symmetrize ranges:

assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W-1:LINE_W],{LINE_W{1'b0}}};

formats like this:

assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W-1:LINE_W], {LINE_W{1'b0}}};

[fangism]

You can leave declarative dimensions as-is.

I re-tested top-of-tree with:

module m;
assign lookup_addr_aligned = {lookup_addr_ic0[  ADDR_W  -  1  :  LINE_W  ],{LINE_W{1'b0}}};
endmodule : m

and got

module m;
  assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W - 1 : LINE_W], {LINE_W{1'b0}}};
endmodule : m

I think your change will result in:

module m;
  assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W-1 : LINE_W], {LINE_W{1'b0}}};
endmodule : m

Can you confirm?

If so, that looks ok to me.
In the long term, this is potentially something users may want to configure, but we haven't worked out a configurability story yet.

I can imagine users may have different opinions on spacing in contexts like this.

I'll comment in the PR.

[ghost]

Not exactly. My changes 'forces' compaction of the whole dimension range expression. So for input like:

module m;
assign lookup_addr_aligned = {lookup_addr_ic0[  ADDR_W  -  1  :  LINE_W  ],{LINE_W{1'b0}}};
endmodule : m

I got:

module m;
  assign lookup_addr_aligned = {lookup_addr_ic0[ADDR_W-1:LINE_W], {LINE_W{1'b0}}};
endmodule

I assumed that intentention of this issue is to force compaction of the whole expression and not just a binary operators.

[fangism]

For now, I suggest limiting it to binary operators. Unary operators are already compact.

What else is left? call like expressions like $builtin(a, b).

It's arguable whether : is a binary operator because it means "range". I suggest leaving spacing around : as-is (unaffected) for now.