VHDL: Slicing and range attributes incorrectly identified as function calls.

[mewais]

Hi Nic,

CASE1:

This example is part of the standard library numeric_bit-body.vhdl, specifically from lines 2774 to 2781.

  procedure READ(L : inout LINE; VALUE : out UNSIGNED; GOOD : out BOOLEAN) is
    variable ivalue : BIT_VECTOR(value'range);
  begin
    READ (L     => L,
          VALUE => ivalue,
          GOOD  => GOOD);
    VALUE := UNSIGNED(ivalue);
  end procedure READ;

the json output for the variable definition is:

{
    "__class__":"HdlIdDef",
    "position":[
        2775,
        14,
        2775,
        19
    ],
    "name":{
        "__class__":"str",
        "val":"ivalue"
    },
    "is_latched":true,
    "type":{
        "__class__":"HdlOp",
        "ops":[
            "BIT_VECTOR",
            {
                "__class__":"HdlOp",
                "ops":[
                    "value",
                    "range"
                ],
                "fn":"APOSTROPHE"
            }
        ],
        "fn":"CALL"
    },
    "direction":"INTERNAL"
}

you can see that the variable definition gets incorrectly identified as a function call. The function name in this case is the datatype which is BIT_VECTOR while the operand is the range which is value'range.
The correct output should be a fn: INDEX with the datatype as operand 0 and the APOSTROPHE as operand 1.

CASE2:

The same issue happens in the left hand side of some assignments. This is an example from file numeric_bit-body.vhdl from lines 160 to 164:

      if TEMP(TOPBIT+J+1 downto J) >= "0"&DENOM(TOPBIT downto 0) then
        TEMP(TOPBIT+J+1 downto J) := (TEMP(TOPBIT+J+1 downto J))
                                     -("0"&DENOM(TOPBIT downto 0));
        QUOT(J) := '1';
      end if;

The left hand side of the assignment is mistakenly identified as a function call rather than INDEX. As follows:

{
    "__class__":"HdlStmAssign",
    "labels":[
        
    ],
    "src":{
        "__class__":"HdlValueInt",
        "val":"1",
        "base":256
    },
    "dst":{
        "__class__":"HdlOp",
        "ops":[
            "QUOT",
            "J"
        ],
        "fn":"CALL"
    },
    "is_blocking":true
}

Thanks Nic.

[Nic30]

Hello,
if I read correctly there is only a single issue.
The index operator is being resolved as a call operator?
I am correct?

I see two problems related to this issue:

1. In some cases it is impossible to resolve if operator is index or call simply because VHDL uses same operator and the symbol which is operator used on has to be resolved in order to find out if this an index or call operator.

2. In CASE2 (mentioned in this issue) the assignment destination can be only a index operator.

[mewais]

Hi Nic,

you're correct. The two cases show one problem, different places and conditions though.

1. That may be true in some cases but not in this case. The type of a signal/constant/variable cannot be a function call. It must be explicitly a type name. so CASE1 should be possible to distinguish.
2. You are correct. For CASE 2 it can only be INDEX, never a function call. So this one may be easy to address.

[mewais]

That said, if it proves too hard to fix, then maybe we can just accept it the way it us and handle it ourselves outside hdlConvertor.

[Nic30]

Correct me if I am wrong but I think that the cases where we can not resolve if this () operator is index or call are only the cases where the operator could be the call. That means we can just use index operator everywhere by default. (instead of call operator)

[mewais]

If you mean by scope then yes.

Type declarations, Signal type definitions, assignment destinations all must have INDEX, not CALL.

Only values of signal definitions, right hand sides of assignments can have it as CALL (or INDEX still, hard to distinguish)

[Nic30]

I run tests of all our tools based on hdlConvertor and all seems to be pasign because the check already had to be performed. I hope that everyone else does have this check as well, because this could be ultimate breaking change.

[mewais]

Thanks Nic, I can confirm this works.

The way I see it, there can still be CALLs instead of APOSTROPHEs sometimes, you just decreased the probability of this happening but didn't completely remove it (as you said, no one can until we look into it). So whoever doesn't have checks for this ran the risk of having bugs anyway.

[mewais]

Hi Nic,

I think you mistakenly included values of signal declarations into this. For example:

CONSTANT SHIFT_AMOUNT_SIZE 			: NATURAL 						:= LOG2(DOUBLEWORD_SIZE)

gets converted to INDEX rather than CALL. This may actually be breaking to existing users.

Signal type declaration should always be INDEX. While signal value declarations can be CALL or INDEX. although I think they should default to CALL for compatibility with existing user base.

[mewais]

I think if this proves too hard to fix, then undoing your changes maybe the best option.

[Nic30]

Hello,

for

CONSTANT SHIFT_AMOUNT_SIZE 			: NATURAL 						:= LOG2(DOUBLEWORD_SIZE)

the LOG2() is represented as index intentionally.
It is impossible to precisely resolve because we do not know that LOG2 is a function.

However there could be a relatively simple transformation which resolves this, but the problem is that we need perform imports, which may require elaboration.

I see it, there can still be CALLs instead of APOSTROPHEs sometimes

Can you provide an example?

[mewais]

Hi Nic, you are correct. I just meant that this is different from what you had before.

Before, there was ONLY the possibility of having INDEX incorrectly identified as CALL. The fix I suggested above should've decreased the probability of such thing happening by handling specific cases (destinations of assignments, type of signal declaration). But as you said, we could never get rid of it completely. That's also not breaking to users, because they should've already been handling this case.
With your fix, you introduced the possibility of having CALL incorrectly identified as INDEX. This will be truly breaking to users since AFAIK it didn't happen before.

What we should have is the following:

1. Assignment destinations: Always INDEX
2. Assignment sources (or any expression operands): could be CALL or INDEX, hdlConvertor should default to CALL for backward compatibility and leave to Elaboration to decide.
3. Signal declaration
   1. Type of signal: Always INDEX
   2. Value of constant/variable: could be CALL or INDEX, hdlConvertor should default to CALL for backward compatibility and leave to Elaboration to decide.

That should be non breaking to users, and would also get rid of the problem for some of the cases.

[mewais]

Hi Nic, any updates about this?

[Nic30]

Hello,
situation is not nice.

If the CALL is a default and INDEX is only in cases you you mentioned, we need to always translate the expression or we need to pass some flag deep in to parsing functions.

I have looked at several libraries which are using this library and everywhere was check in the format "== CALL or == INDEX", because it was mixed before. So I expect that this will not have any ridiculous implications. Because the check for this is probably already in correct format. (But of course, this is just my speculation. And I am more or less just waiting if some user complains in this issue.)