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.

[ubaidsk]

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.

[ubaidsk]

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.

[ubaidsk]

@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
}