alchitry / Alchitry-Labs-V2

Full rewrite of Alchitry Labs
https://alchitry.com/
GNU General Public License v3.0
14 stars 2 forks source link

binToDec component bug on sub_value sig and innermost repeat index #17

Closed natalieagus closed 4 weeks ago

natalieagus commented 1 month ago

I tried to use binToDec component and met this syntax error:

image

Since it's a component file and read-only, I can't directly modify it. The error in binToDec.luc won't show up unless we instantiate DIGITS, e.g: in alchitryTop.luc:

binToDec binToDecConverter(#DIGITS(4))

Attempt to fix

I tried to move the definitions of the signals outside and initialise their values (I call this binToDecV1), and it works well on simulation, but not on compilation.

module binToDecV1 #(
    DIGITS ~ 1 : DIGITS > 0 && DIGITS < 20,           // limited by 64 bit constants in the tools
    LEADING_ZEROS = 0 : LEADING_ZEROS == 0 || LEADING_ZEROS == 1
)(
    input value[$clog2($pow(10, DIGITS))],            // minimum number of bits for DIGITS 
    output digits[DIGITS][4]                          // decimal output
) {

    sig scale[$width(value)]
    sig remainder[$width(value)]                         // running remainder
    sig sub_value[$width(value)]                         // temporary subtraction value
    sig blank                                          // flag for leading zeros

    always {
        repeat(i, DIGITS){
            digits[i] = d11                                // default to invalid number
        }

        remainder = value                                // initialize remainder
        blank = !LEADING_ZEROS                           // set blank zero flag
        scale = 0
        sub_value = 0

        if (value < $pow(10, DIGITS)) {                   // if can be displayed
            repeat(j, DIGITS, DIGITS-1, -1){// for each digit  
                scale = $pow(10, j)                          // get the scale for the digit

                if (remainder < scale) {                      // if this digit is 0
                    if (j != 0 && blank)                        // use 10 for blank
                        digits[j] = 10
                    else                                        // or 0 for zero
                        digits[j] = 0
                } else {                                      // digit is 1-9
                    blank = 0                                  // don't blank future zeros
                    sub_value = 0                              // default to no subtraction
                    repeat(i, 9, 9, -1){// for each possible value (starting from 9)

                        if (remainder < (i+1) * scale) {          // if remainder is less than value
                            digits[j] = i                          // set digit to this value
                            sub_value = i * scale                  // set subtraction value
                        }
                    }
                    remainder = remainder - sub_value          // subtract off last digit
                }
            }
        }
    }
}

This is the bug reported during build, something about "Function "pow" should always be constant but didn't have a value!"

image

Here's the problematic line:

image

It was fine for build in Lucid V1, line 57.

Other bugs

The need for "instantiating" scale and sub_value?

If I were to comment out these two lines to match bin_to_dec.luc in Lucid V1:

    always {
        repeat(i, DIGITS){
            digits[i] = d11                                // default to invalid number
        }

        remainder = value                                // initialize remainder
        blank = !LEADING_ZEROS                           // set blank zero flag

        // COMMENT THIS OUT to match V1 library 
        // scale = 0
        // sub_value = 0

I will get this error:

image

I thought that since scale and sub_value is always defined whenever it is used, it shouldn't give out this error.

Innermost repeat index bug

The original binToDec.luc component has this clause towards the end,

image

I believe it should be repeat(i, 9, 9, -1) to loop for each possible value (starting from 9).

alchitry commented 4 weeks ago

This is actually three bugs.

First, you are correct. The loop count should be 9.

Second, the initial error was a bug in the driver check where it would look only at the most recent write to a signal instead of all writes. Since sub_value was conditionally written in an if statement, it thought it wasn't guaranteed to have a value even though it has a default value.

These two were fixed in commit https://github.com/alchitry/Alchitry-Labs-V2/commit/6ffd35331f5136df71971a5fc72258da90efa176

The last issue is a bigger one and I opened a new bug report, #18, with it.