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

Can't use for loop variable in expression in if_reset statement #964

Closed
nananapo opened this issue Sep 13, 2024 · 14 comments · Fixed by #1027
Closed

Can't use for loop variable in expression in if_reset statement #964

nananapo opened this issue Sep 13, 2024 · 14 comments · Fixed by #1027
Labels
bug Something isn't working

Comments

@nananapo
Copy link
Contributor

module main (
    clk: input clock,
    rst: input reset
)
{
    var r : logic<32,32>;
    always_ff{
        if_reset {
            for i : u32 in 0..32{
                r[i] = i;
            }
        }
    }
}
�]8;;https://doc.veryl-lang.org/book/07_appendix/02_semantic_error.html#invalid_reset_non_elaborative�\invalid_reset_non_elaborative (link)�]8;;�\

  💥 Reset-value cannot be used because it is not evaluable at elaboration
  │ time
    ╭─[:9:24]
  8 │             for i: u32 in 0..32 {
  9 │                 r[i] = i;
    ·                        ┬
    ·                        ╰── Error location
 10 │             }
    ╰────
  help: 
@dalance dalance added the bug Something isn't working label Sep 13, 2024
@nblei
Copy link
Contributor

nblei commented Sep 13, 2024

I think we do not have a mechanism to identify that the i is a loop value through a fixed-range loop.

In the meantime, nananapo, you can use a generate loop which contains an always_ff block.

@taichi-ishitani
Copy link
Contributor

image

genvar is also treated as a variable.
It should be constant value.

@dalance
Copy link
Collaborator

dalance commented Sep 25, 2024

I think genvar is clearly constant variable, but wonder whether loop variable is constant or variable.
In the following code, the value of i at each loop is fixed at elaboration in the both cases.
So should i be treated as constant? Is there any case which i should be treated as variable?

    always_ff{
        if_reset {
            for i : u32 in 0..32{
                r[i] = i;
            }
        }
    }

    var N: u32;
    always_ff{
        if_reset {
            for i : u32 in 0..N{
                r[i] = i;
            }
        }
    }

@taichi-ishitani
Copy link
Contributor

taichi-ishitani commented Sep 25, 2024

For SystemVerilog, following code causes elaboration error because operand of part selection must be constant.

for (int i = 0;i < W;++i) begin
  a[i:0] = i;
end

Therefore, I think Veryl should treat a loop variable as a variable except when it is referred as a reset value.

@dalance
Copy link
Collaborator

dalance commented Sep 25, 2024

I think Veryl should treat a loop variable as a variable except when it is referred as a reset value.

I think this rule is bit difficult, and may introduce ambiguous edge cases in the future. "genvar is constant, and loop variable is variable" is simple and sufficient?

@taichi-ishitani
Copy link
Contributor

taichi-ishitani commented Sep 25, 2024

Instead of this, how about removing reset value check?

@dalance
Copy link
Collaborator

dalance commented Sep 27, 2024

Even if the check is removed, a[i:0] issue still remains.
I think changing genvar to constant is correct at least, and it resolves this issue by generate loop.

@taichi-ishitani
Copy link
Contributor

I read before an article that reducing number of procedural blocks such as always block is good technique to improve simulation performance.
Therefore, I think it's better to process an array within one always block but not multiple always blocks using generate for loop.

@dalance
Copy link
Collaborator

dalance commented Sep 27, 2024

OK. I'll merge #974, but leave this issue.

@taichi-ishitani
Copy link
Contributor

taichi-ishitani commented Sep 29, 2024

Even if the check is removed, a[i:0] issue still remains.

Regular variables also cause this error so we need to introduce an additional check.
For this check, we can treat a loop variable as a regular variable and then don't need to treat it as a special variable.

@taichi-ishitani
Copy link
Contributor

Even if the check is removed, a[i:0] issue still remains.

Regular variables also cause this error so we need to introduce an additional check. For this check, we can treat a loop variable as a regular variable and then don't need to treat it as a special variable.

I think this check can be done by #788.

@dalance
Copy link
Collaborator

dalance commented Oct 3, 2024

I think #788 is not so easy.
If ConstantExpression: Expression are added, how definition of Select is reasonable?
Probably the following is avaibable, but these many duplications occur whole syntax.

Select: LBracket Expression [ SelectOperator Expression ] RBracket;
ConstSelect: LBracket ConstExpression [ SelectOperator ConstExpression ] RBracket;

@taichi-ishitani
Copy link
Contributor

taichi-ishitani commented Oct 4, 2024

There are 4 patterns of select operation.

a = X[0];        // bit select
b = X[1:0];      // select by msb and lsb
c = X[1+:2];     // select by position and width
d = X[1 step 2]; // select by step

For msb and lsb pattern both of 1st and 2nd expressions must be constant.
On the other hand, for other patterns 1st expression can be variable.
So we need to consider how to handle msb and lsb pattern.

I think we have following choises.

  1. Remove msb and lsb pattern
  2. Change the rule like LBracket Expression [ SelectOperator ConstExpression ] RBracket and insert the additional check for msb and lsb pattern
  3. Introduce special notation for msb and lsb pattern

@dalance
Copy link
Collaborator

dalance commented Oct 4, 2024

I'm starting to feel that introducingConstExpression is not so good idea.
The idea of ConstExpression means that whether expression is const or not can be determined syntactically.
Of course, sometimes it can be, but not always (e.g. reset value).
So the additional check for some Expression is reqruired even if ConstExpression is introduced.

I think inserting const check to Expression which is syntactically const is easy, and the place which is ambiguous is difficult regardless of introducing ConstExpression.
So I think ConstExpression doesn't has enough effect to reduce analyzer complexity.
Maybe inserting const check to each Expression is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants