veryl-lang / veryl

Veryl: A Modern Hardware Description Language
Other
476 stars 22 forks source link

Veryl does not check if the given expression is constant #652

Open taichi-ishitani opened 5 months ago

taichi-ishitani commented 5 months ago

There are some cases that the given expression should be constant, e.g. expression for generate block. But Veryl does not check such cases and report no errors.

image

taichi-ishitani commented 2 months ago

Veryl also reports no errors for the assignments below.

module ModA {
  let a: u32 = 1;
  local A: u32 = a;
}

module ModB {
  function FuncB (
    b: input u32
  ) -> u32 {
    return b;
  }

  let b: u32 = 2;
  local B: u32 = FuncB(b);
}

module SubC #(
  param C: u32 = 1
) {}

module TopC {
  let c: u32 = 3;
  inst u_sub: SubC #(
    C: c
  )();
}
taichi-ishitani commented 2 months ago

Need to check if the function call on the RHS is constant.

taichi-ishitani commented 2 months ago

Need to check if the function call on the RHS is constant.

Condition of constant function call.

image

nblei commented 2 months ago

For ModA --- I think this is fine. let a = 1 is binding a elaboration time constant to a, which is then assigned to a local. While this may need to transpile to slightly different SV than what it does currently, there's no reason in general that it should not be allowed. Similarly, with the TopC and SubC. In the Type struct, there's an is_const flag. Perhaps an is_elaborative flag would be useful, as well.

As you've noted, identifying constant functions is a bit complex, and currently not done. That would be a significant improvement.
There is some trickiness, though. Synopsys will elaborate and synthesize recursive and mutually recursive functions. The constant function requirements also do not specify anything about recursion limits. Thus, according to the doc, halting is not required for a constant function. So the two approaches would be to 1) simply build a graph whose nodes are functions and whose edges are function calls which occur in those functions and declare non-const if any non-const function is reachable, or 2) for all elaboration time invocations of the function, actually execute the function (to some recursion limit due to halting problem) and declare the function as const iff all elaboration time invocations result in a constant expression. For functions which occur in public, parametric modules/packages/etc, this would require symbolic execution.

I am also of the opinion that C++ does this in a much better way, with the required constexpr keyword if you want your function to be evaluable at compile time. This simplifies approach 1 --- a function is non-const if there exists a reachable function not marked as constexpr.

taichi-ishitani commented 2 months ago

While this may need to transpile to slightly different SV

You mean that Veryl should transpile a let statement into a localparam declaration if its RHS is constant?

nblei commented 2 months ago

Yes, I think so. But I also think we should use two different words: "constant" to describe an immutable variable binding and "elaborable" to describe a constant which is determined during hardware elaboration --- similar to the difference between const and constexpr in C++. Thus, I think it should transpile to a locaparam when the RHS is elaborable.

The other option is to not do that and give an error saying that a let binding is constant but not elaborable and they should use a localparam instead.

taichi-ishitani commented 2 months ago

we should use two different words: "constant" to describe an immutable variable

What is difference between constant declaration and local declaration?

nblei commented 2 months ago

Good question, and I do not think we should have an explicit constant qualifier.

SV allows a const var declarations, but veryl does not have a const qualifier - let already does that.

Further, the notion of constness outside of procedural blocks is a bit murky.

Is let a : logic = i_a; 'const'? It is a variable, a, which is bound to i_a. So in a sense, it is like a logic const * from C or a reference from C++ --- it is forever bound to refer to whatever value i_a takes.

On the other hand, inside a procedural block, i_a's value can never change, since a procedural block executes in zero simulation time. Thus,

always_<comb|ff> (
  let a = i_a;
);

is const in the normal sense of the word --- the variable a is bound to the value of i_a for the entirety of the execution of the procedural block. Recall @dalance added the let binding specifically for use in always_ff blocks --- because otherwise always_ff assignments defaulted to non-blocking.

But even here this is drastically different from local. In a local a: logic = ? binding, the value bound to a can never be changed. Thus, local a: logic = i_a is an error. Ignoring for the moment the existence of x and z, a can be thought of as a list of tie-up and tie-down standard cells --- if you require logic more complicated than tie-ups and tie-downs, it is not an elaboration time constant. So it is equivalent to C++ constexpr int a = ?;. Only a value which is known during elaboration/compilation can be assigned to a variable declared with a local binding.

taichi-ishitani commented 2 months ago

I think it is confusable that the characteristic of the let binding (variable or elaboration time constant) is changed according to its RHS.

The other option is to not do that and give an error saying that a let binding is constant but not elaborable and they should use a localparam instead.

So I agree with this option.