#47 verilog function argument can be incorrect

closed
nobody
None
5
2014-12-15
2012-08-21
Anonymous
No

The first argument of a functions gets overwitten (changed) when the second argument of that function is the return of anther invocation of that same function. Thus:

func(arg1, func(arg2, arg3)) becomes func(arg2, func(arg2, arg3))

The attached file compiles but it gives incorrect results.

iverilog function-tb.v ; ./a.out

Discussion

  • Add numbers and get the wrong result.

     
    Attachments
  • Cary R.
    Cary R.
    2012-08-21

    I only tested this in development so far. Making the functions automatic solves the problem. For non-automatic functions we may be able to assume a call order, but I'm not sure it is guaranteed so both functions could be active at the same time which would lead to the problem shown.

    On a related note. If the display is commented out I would assume these should be interpreted as constant functions. That did not appear to happen.

     
  • This is a slightly grey area. The standard clearly states that function arguments can be evaluated in any order, but it does not state when the values should be assigned to the function input variables. Icarus assigns the values as it evaluates them, whereas another simulator I tried evaluates all the arguments first, then assigns them. Absent anything in the standard to the contrary, both would seem to be legal behaviour.

    On the related note, constant functions are currently only evaluated at compile time if they occur in an expression that is required to be constant. I did put in place the infrastructure necessary to allow compile time evaluation wherever possible, but it is not enabled at present.

     
  • Cary R.
    Cary R.
    2012-08-21

    I think by just reordering the compiler output code (tgt-vvp changes) we could make this evaluate then assign and to me that personally makes more sense. I agree it is not clearly defined so is not required. I would also say if the big three and possibly most importantly NCVerilog do this as evaluate then assign then we should match them.

    Thanks for the clarification on the current constant function implementation.

     
  • Cary R.
    Cary R.
    2012-08-22

    As I posted to the iverilog-devel mailing list both gplcver and veriwell work like Icarus currently does. I have requested that this code be tested on other simulators to see how the big three or other more modern implementations work. If we get a consensus from this testing or if key simulators perform differently we may change this functionality.

    Like Martin has already stated this is not a bug in Icarus only a different interpretation of the standard.

     
  • Evaluate then assign would be more user-friendly, and would save the overhead of using automatic functions for this use case. I've looked at the tgt-vvp code, and think it would be fairly easy to implement. The downside would be that it would increase the number of thread bits or word registers used. This could be a significant issue for functions with a lot of real arguments, as a thread only has 16 word registers.

     
  • Cary R.
    Cary R.
    2012-08-22

    I would assume for automatic functions we do not want to use the evaluate then assign scheme since if we directly assigned to them, which is safe, they can be used if the normal evaluation process exhausted the word registers. I'm guessing the code generator could detect this and tell the user to switch to automatic functions and why. I also agree that evaluate then assign while less memory efficient is probably more in line with what the user would expect.

     
  • Cary R.
    Cary R.
    2012-08-22

    I'll also add that we need to get input from Steve to see if this is an acceptable change for V0.9. Since it is a rare corner case it may be acceptable.

     
  • Cary R.
    Cary R.
    2012-08-23

    • milestone: 896955 -->
    • labels: 776822 -->
     
  • Cary R.
    Cary R.
    2012-08-23

    I just talked with Steve and we are not going to fix this in V0.9 because of the limitation regarding overflowing real arguments and because the current functionality does not violate the standard. For now this can be worked around by making the function automatic.

    I'm going to move this to the feature request tracker and it will be addressed in development since we all agree this is the preferred functionality. There are plans to switch the procedural operators to a stack based system at some point in time. Once that is done the evaluate then assign will come by default (evaluate an argument and push it onto the stack then assign all the stack values to the appropriate function variable).

     
  • Cary R.
    Cary R.
    2012-08-23

    • milestone: --> devel_(Next_Release)
     
    • status: open --> accepted
     
  • I've implemented this change in the master branch. The example provided in this feature request now produces the expected result.

     
    • status: accepted --> closed