Wrong formatting of constraints blocks

[msfschaffner]

Verible erroneously reformats the following blocks into one-liners.

before:

constraint c_iv {
    if (fixed_iv_en) {
      aes_iv == fixed_iv
    };
  }

  constraint c_operation {
     if (fixed_operation_en) {
         aes_operation == fixed_operation
     };
  }

after:

constraint c_iv {if (fixed_iv_en) {aes_iv == fixed_iv};}

constraint c_operation {if (fixed_operation_en) {aes_operation == fixed_operation};}

[msfschaffner]

b/168312418

[vasantteja]

Can I assign this to myself?

[fangism]

Yes, thank you for volunteering!

Tips: examine verible-verilog-syntax --printtree to see the internal syntax tree structure.
verible-verilog-format --show_token_partition_tree (optionally --show_inter_token_info) will show you the formatter internal representation as a hierarchical tree of token ranges.

Feel free to ask any questions to help get you going.

[fangism]

@vasantteja Checking in, need any help or have any questions?

[fangism]

@vasantteja Checking in, need any help or have any questions?

[vasantteja]

@fangism sorry, I was traveling and I couldn't take a look at this. I will reach out to you if I have any doubts. Thanks for the patience.

[ghost]

Hi @msfschaffner,
Just for clarification: all constraints blocks should be expanded?

For example I've found in an Ibex source code such line:

constraint only_vec_instr_c {soft only_vec_instr == 0;}

here: https://github.com/lowRISC/opentitan/blob/master/hw/vendor/lowrisc_ibex/vendor/google_riscv-dv/src/riscv_vector_cfg.sv#L32

and I'm not sure how to procceed.

Current PR expands all blocks except some simple one like that above:
https://github.com/google/verible/pull/706/files#diff-631e76d6700a6682c3bdc7f0aad5aa02a4f117aa2e8ae3e6b96c2ddaf34c92f1R9164

[msfschaffner]

Hey @ldalek-antmicro,

How about we standardize on something that only expands if the expression inside the outermost {} brackets contains more {} blocks or does not fit onto one single line?

[ghost]

Thanks for an answer @msfschaffner

Got one more question about multi item constraints like:

constraint xx {
  aa == bb;
  cc == dd;
}

should them be fitted in one line (like you wrote above) or expanded (IMHO looks better expanded)?

[msfschaffner]

Yes, this also looks better to me in expanded form.

How about we only compact onto one line if the conditions mentioned above hold, AND the brackets only contain one expression?

[ghost]

Okay, to sum up:

1. If the expression contains brackets, expand, .e.g.

constraint xx {
  if (a) {
    b;
  }
}

2. If the expression contains more than two expressions, expand, e.g.

constraint yy {
  a == b;
  c == d;
}

3. If the expression doesn't fit, expand, e.g.

constraint yyyyyyyyyyyyyyyyyyyyy {
  aaaaaaaaaaaaaaaaaaaaaaa == bbbbbbbbbbbbbbbbbbbbbbbbb;
}

4. otherwise, compact, e.g.

constraint only_vec_instr_c {soft only_vec_instr == 0;}

[msfschaffner]

Yes this SGTM!

[sriyerg]

Okay, to sum up:

1. If the expression contains brackets, expand, .e.g.

constraint xx {
  if (a) {
    b;
  }
}

1. If the expression contains more than two expressions, expand, e.g.

constraint yy {
  a == b;
  c == d;
}

1. If the expression doesn't fit, expand, e.g.

constraint yyyyyyyyyyyyyyyyyyyyy {
  aaaaaaaaaaaaaaaaaaaaaaa == bbbbbbbbbbbbbbbbbbbbbbbbb;
}

1. otherwise, compact, e.g.

constraint only_vec_instr_c {soft only_vec_instr == 0;}

How will this get formatted?

  constraint foo_c {
    foo inside {[0:1]};
  }

Looks like the formatter will ignore this?

[hzeller]

That would fit in one line so would be formatted compactly:

$ verible-verilog-format - <<EOF
constraint foo_c {
    foo inside {[0:1]};
  }
EOF
constraint foo_c {foo inside {[0 : 1]};}

[ghost]

But should be expanded (because of brackets).
This PR #761 fixes this. @hzeller can you look at it?

[sriyerg]

But should be expanded (because of brackets).
This PR #761 fixes this. @hzeller can you look at it?

That's what I thought. Its a bit inconsistent then. This should be compacted too because it is not a conditional.

[ghost]

@sriyerg Expression inside constraint block doesn't have to be a conditional to force expansion of such block.
@msfschaffner or does it?

[msfschaffner]

Hm... I believe the question is how rule 1. should be interpreted.

Should the expansion trigger on the conditional, or on the {} brackets.

It looks like conditionals with {} brackets are typically written a bit differently than inside {} statements, right @sriyerg?

If that is the case, shall we change that rule to only expand on conditional {} blocks?

[sriyerg]

My preference TBH is to always expand constraints - i.e. put the expressions on a separate lines. It is the most consistent solution.

  constraint foo_c {
    expr1;
    expr2;
    ...
  }

If the exprs are either conditionals (if (x) { ...), loops (foreach (x[i]) {), constraint distributions ( x dist {), they must be expanded further.