C backend: variable is initialized twice

[certik]

Example:

from ltypes import f64

def f() -> f64:
    return 5.5

def main():
    t1: f64 = f()*1e6
    print(t1)

main()

This generates:

$ lpython --show-c a.py
...
void _xx_lcompilers_changed_main_xx()
{
    double t1 = f()*  1.00000000000000000e+06;
    t1 = f()*  1.00000000000000000e+06;
    printf("%lf\n", t1);
}
...

Which is wrong, it should only be initialized once.

[Shaikh-Ubaid]

I have also experienced this issue with the wat generation.

For example, the same example at #1626 (comment) produces the following wat which has duplicated assignment:

...
(func $4 (type 4) (param) (result)
        (local f64)
        call 3
        f64.const 1000000.000000
        f64.mul
        local.set 0 // first assignment
        call 3
        f64.const 1000000.000000
        f64.mul
        local.set 0 // second assignment
...

I think the issue is at the ASR level. The following example

from ltypes import i32

def main():
    t1: i32 = 5

main()

generates the ASR:

main:
...
(Function
    (SymbolTable
        2
        {
            t1:
                (Variable
                    2
                    t1
                    []
                    Local
                    (IntegerConstant 5 (Integer 4 [])) // symbolic value for the variable
                    ()
                    Default
                    (Integer 4 [])
                    Source
                    Public
                    Required
                    .false.
                )
        })
    main
    (FunctionType
        []
        ()
        Source
        Implementation
        ()
        .false.
        .false.
        .false.
        .false.
        .false.
        []
        []
        .false.
    )
    []
    []
    [(=                                            // assignment for the variable
        (Var 2 t1)
        (IntegerConstant 5 (Integer 4 []))
        ()
    )]
    ()
    Public
    .false.
    .false.
)
...

In the above asr, we see that the variable t1 has a symbolic value of IntegerConstant 5 and in the function body, the same variable is also being assigned the value of IntegerConstant 5. This leads to duplicate assignment of values to variables initialised during declaration.

I think ideally the asr should either contain the symbolic value and no initialisation statements in function body, or it should contain no symbolic value and initialisation statements in function body.

[certik]

Yes, the ASR is wrong. It needs to only do the assignment, and not having the symbolic value set. So that's what we need to fix.

[Shaikh-Ubaid]

For the following example:

from ltypes import i32
from numpy import empty, int64

def main():
    my_arr_len: i32 = 5
    my_arr: i64[my_arr_len] = empty((my_arr_len), dtype=int64)
    print(my_arr_len)
main()

If we ignore the symbolic value, the c code (similar to wat) that would get generated is

int32_t my_arr_len;
struct i64 my_arr_value;
struct i64* my_arr = &my_arr_value;
int64_t *my_arr_data = (int64_t*) malloc(sizeof(int64_t)*my_arr_len);
my_arr->data = my_arr_data;
my_arr->n_dims = 1;
my_arr->dims[0].lower_bound = 0;
my_arr->dims[0].length = my_arr_len;
my_arr_len = 5;
printf("%d\n", my_arr_len);

We see that the array length dimension is assigned the value of my_arr_len even before my_arr_len gets value assigned.

We would have an undefined dimension during array declaration. The dimension value would be available after the array has been declared. How do we solve this?

[certik]

I am raising to high priority, this is a blocker for several projects and a big bug causing random segfaults when you do:

        a: f64 = p.x

The struct p is not initialized at the point when a is declared, and it segfaults randomly as a result.

[Smit-create]

I'm taking a look into it. Maybe one of the fixes is to look at the dependency list.

[Shaikh-Ubaid]

@certik I updated the comment #1626 (comment). Please have a look.

I had applied the following change to generate the c code in #1626 (comment).

--- a/src/lpython/semantics/python_ast_to_asr.cpp
+++ b/src/lpython/semantics/python_ast_to_asr.cpp
@@ -2218,8 +2218,7 @@ public:
                                                         init_expr, nullptr);
             current_body->push_back(al, ASRUtils::STMT(assign));
             ASR::Variable_t* v_variable = ASR::down_cast<ASR::Variable_t>(v_sym);
-            if( !ASR::is_a<ASR::Const_t>(*type) &&
-                ASRUtils::is_aggregate_type(type) ) {
+            if( !ASR::is_a<ASR::Const_t>(*type)) {
                 v_variable->m_symbolic_value = nullptr;
                 v_variable->m_value = nullptr;
                 Vec<char*> variable_dependencies_vec;

[certik]

There are probably multiple bugs related to this. There are some bugs in dependencies. Then there is a bug in ASR that we set both he m_value and do an assignment later.

[certik]

Related to dependencies: it's not just declaring the the variables in the right order. It's also the fact that the RHS has side effects, so it must be executed exactly where it was declared in Python, not earlier. Conclusion: unless it is a Const, we should never set m_value, but always create a regular Assignment at the place where it was declared, which should fix everything.

[czgdp1807]

The problem is that to deal with initialisation at declaration we convert it to assignment statement in the body. Now ideally such things shouldn't be done. We should remove such conversions and only do initialisation at declaration in all the backends. That would be ideal case.

[certik]

I think we must be free to "declare" in any order (obeying the dependencies), before any executable statements. Consequently, the only expressions allowed in initializations are such that do not depend on executable statements and runtime values. So essentially compile time constants. And this should be checked in verify(). Then the frontend can decide how to initialize, but it can't use runtime expressions in initializers, as that fundamentally can't work.

[czgdp1807]

Then we are required to do the following things,

1. Dis-allow runtime initialisation expressions in both LPython and LFortran. Add a check in ASR verify and also in an assert statement in all the backends especially at this line in the LLVM backend. So calls like f() * 1e6 will also become invalid (until we have an ASR interpreter that figures out if a function is just actually returning a constant or specify the return type as Const[f64] where its clear that value will be a constant at all times). Cases like, integration_tests/variable_decl_01.py will also not work. Why because, l: i32 = 2 is being used in a: i64[n, m, l]. So you just need to extract the Variable::m_value attribute from l and use it directly in a: i64[n, m, l].
2. Do not consider m_symbolic_value while doing initialisation at declaration in any backend in both LPython and LFortran. Only use, m_value because we know everything is a compile time constant.
3. If you do only assignments and don't consider Variable::m_value then LFortran won't work because it does initialisation at declaration correctly (and it doesn't need to convert these to assignments in the body like LPython does).

So a suggestion would be that if you are making any change in backend then @Smit-create please test it with LFortran as well.

Note the LLVM output as well,

define void @__module__global_symbols__xx_lcompilers_changed_main_xx() {
.entry:
  %t2 = alloca double, align 8
  store double 7.000000e+00, double* %t2, align 8
  %t1 = alloca double, align 8
  %0 = call double @__module__global_symbols_f()
  %1 = fmul double %0, 1.000000e+06
  %2 = load double, double* %t2, align 8
  %3 = fadd double %1, %2
  store double %3, double* %t1, align 8
  store double 7.000000e+00, double* %t2, align 8
  %4 = call double @__module__global_symbols_f()
  %5 = fmul double %4, 1.000000e+06
  %6 = load double, double* %t2, align 8
  %7 = fadd double %5, %6
  store double %7, double* %t1, align 8
  %8 = load double, double* %t1, align 8
  call void (i8*, ...) @_lfortran_printf(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @2, i32 0, i32 0), double %8, i8* getelementptr inbounds ([2 x i8], [2 x i8]* @1, i32 0, i32 0))
  br label %return

return:                                           ; preds = %.entry
  ret void
}

[certik]

1. Yes. I think that is the only correct and robust way to do it.
2. We should use m_value, but why would m_symbolic_value be wrong? See my comment here: Bug in variable initialization #651 (comment), the m_symbolic_value should be equivalent to m_value, just symbolic (like x+1, where x is a compile time constant). So the backend can use either one.
3. Yes. If there is m_value, then there cannot also be the same assignment in ASR (currently we apparently do both in LPython, which is a bug we need to fix). If there is no m_value, then there should eventually be an assignment somewhere to initialize the variable.

[czgdp1807]

3. (currently we apparently do both in LPython, which is a bug we need to fix)

There are tests like integration_tests/variable_decl_01.py which forced me to do both. But anyways there is a way to avoid it. If we disallow runtime expressions then we won't need to worry. Now, test cases like #651 are also problematic. @Smit-create just keep this in mind while working on the fix.

[certik]

Yes, the test case integration_tests/variable_decl_01.py is indeed wrong.

The new rules are:

• Keep m_symbolic_value and m_value in sync, but only use them for compile time expressions and only for constants, never for a runtime expression, and never for a regular non-const variable. Check that if m_value (and m_symbolic_value) is set, then this variable is never assigned to, as it is a constant.
• If a runtime expression is used in the Python code, or the variable is not a constant, then we must generate an assignment, and not set any m_symbolic_value nor m_value.

In integration_tests/variable_decl_01.py we have:

def f(n: i32, m: i32):
    l: i32 = 2
    a: i64[n, m, l] = empty((n, m, l), dtype=int64)

In here l = 2 is a compile time expression, however l is not a constant, so m_value should not be used and consequently a: i64[n, m, l] must give a compiler error (we should add a test for this error). To fix the test, just change l: i32 = 2 to l: Const[i32] = 2, then it will work, as l is a constant and 2 is a compile time expression (as required by Const too), and it can then be used in a: i64[n, m, l].

In #651 we have:

def f():
    a: i32 = 5
    x: i32 = 3
    x = 5
    b: i32 = x+1
    g(a*b+3)

Here there are no Const, so none of these variables will use m_value. No problem.

If we don't already have it, also add a test for:

    x: Const[i32] = 3
    x = 5

And ensure it gives a compiler error at x=5 that one cannot assign to a constant.

Document in ASR documentation that m_symbolic_value and m_value must either be both set or both not, and if they are set, then the Variable is a constant, if they are not set, the variable is not a constant. We might later split it and use a dedicated node for that, but for now we will continue using Variable for constants.

Finally, the C (and future Python) backends can declare a variable at first assignment, or they can choose to declare all variables first. We will lose the information if the user declared the variable at first use, or before that, but I think that's not a problem, since we can add either some heuristics or compiler options for the backend to do the natural thing that the user wants.

[Smit-create]

How about just removing the extra assignment and fixing the LLVM, and C backend to initialize (or add assignment statement, etc.) while declaring the variable using v.m_value if present or v.m_symbolic_value. I think this way, we won't break any of the current code after some fixes in the backend?

[Shaikh-Ubaid]

I'm curious to know if there are any specific advantages to having both m_value and m_symbolic_value attributes, or if it would be possible to simplify the code by using only one of them.

Keep m_symbolic_value and m_value in sync

Since, both the attributes would be in sync, it seems one of them could be redundant.

[Thirumalai-Shaktivel]

How about just removing the extra assignment and fixing the LLVM, and C backend to initialize (or add assignment statement, etc.) while declaring the variable using v.m_value if present or v.m_symbolic_value. I think this way, we won't break any of the current code after some fixes in the backend?

I think, this might cause Segfault in LLVM. I have seen this in the global variables (I don't know how's the case inside a function).

[czgdp1807]

How about just removing the extra assignment

That won't work. #651 will create problems. You need to have the extra assignments, that's why I highlighted it in #1626 (comment).

Since, both the attributes would be in sync, it seems one of them could be redundant.

No. When converting back to original source code (like ASR to Fortran or ASR to Python), you will need m_symbolic_value. However in backends like, C, LLVM where originality of the code doesn't matter much, you can ignore m_symbolic_value and always use m_value.

2. So the backend can use either one.

I dis-agree. Backend should always use m_value. Why? Because m_symbolic_value always needs to be a compile time constant which is stored in m_value. So using m_value directly in backends like C, LLVM make sense. If m_value is nullptr then that means m_symbolic_value doesn't reduce to a compile time constant so backend should raise an error.

Also, double initialisation will always happen in LPython (in order to support #651). However by using m_value you won't face issues like segmentation fault. Because m_symbolic_value is converted to an assignment statement which happens when all the variables are declared. And m_value will be used while doing initialisation at declaration which is a compile time constant so no segmentation faults as we aren't using any other variable for initialisation just a compile time constant.

[czgdp1807]

• If a runtime expression is used in the Python code, or the variable is not a constant, then we must generate an assignment, and not set any m_symbolic_value nor m_value.

Well then double initialisation will also not happen because you are removing m_symbolic_value, m_value by setting them to nullptr but then we will loose the information for converting ASR back to original source code (which is okay). But note that you can't have m_value != nullptr and m_symbolic_value == nullptr.

[czgdp1807]

@Smit-create Please read the above comments and follow #1626 (comment) for doing things.

[certik]

You can't have m_value != nullptr and m_symbolic_value == nullptr, and I suggest you can't have m_symbolic_value != nullptr and m_value == nullptr either.

If m_value is nullptr then that means m_symbolic_value doesn't reduce to a compile time constant so backend should raise an error.

If m_value is nullptr, then also m_symbolic_value is a nullptr.

Although there is an issue of save (static) variables in Fortran. We might need to brainstorm this case more.

by setting them to nullptr but then we will loose the information for converting ASR back to original source code (which is okay).

See the last paragraph in #1626 (comment) that addresses this.

[Smit-create]

In here l = 2 is a compile time expression, however l is not a constant, so m_value should not be used and consequently a: i64[n, m, l] must give a compiler error (we should add a test for this error). To fix the test, just change l: i32 = 2 to l: Const[i32] = 2, then it will work, as l is a constant and 2 is a compile time expression (as required by Const too), and it can then be used in a: i64[n, m, l].

If we are allowing n, and m as arguments that are not constant, shouldn't we also allow l? Because the dimensions are going to be evaluated at the runtime provided n and m both are non-const variables.

[Smit-create]

@czgdp1807 Can you please try out the suggestion #1626 (comment)? Thanks

[certik]

The example above is fixed now:

...
void _xx_lcompilers_changed_main_xx()
{
    double t1;
    t1 = f()*  1.00000000000000000e+06;
    printf("%lf\n", t1);
}

Thanks @czgdp1807 and @Smit-create for quickly fixing it!