Do not load CPtr #2440

certik · 2023-12-06T18:21:04Z

No description provided.

anutosh491 · 2023-12-08T04:59:52Z

This seems to be the correct fix for the concerned program

from lpython import S
from sympy import Symbol

def mmrv(x: S) -> list[S]:
    l1: list[S] = [x]
    return l1

def test_mrv1():
    x: S = Symbol("x")
    ans: list[S] = mmrv(x)

test_mrv1()

But fails for symbolics_11.py (actually just the first 2 lines)

anutosh491 · 2023-12-08T05:05:57Z

Consider the following

from sympy import pi
from lpython import S

def test_extraction_of_elements():
    l1: list[S] = [pi]

test_extraction_of_elements()

The above change would disturb this simple block that works on master. I think there's a type mismatch happens while storing elements into the list

define void @__module___main___test_extraction_of_elements() {
.entry:
  %_stack0 = alloca i64, align 8
  %l1 = alloca %list, align 8
  %0 = call i8* (i32, ...) @_lfortran_malloc(i32 8)
  %1 = bitcast i8* %0 to void**
  %2 = getelementptr %list, %list* %l1, i32 0, i32 2
  store void** %1, void*** %2, align 8
  %3 = getelementptr %list, %list* %l1, i32 0, i32 0
  store i32 0, i32* %3, align 4
  %4 = getelementptr %list, %list* %l1, i32 0, i32 1
  store i32 1, i32* %4, align 4
  %stack0 = alloca void*, align 8
  store i64 0, i64* %_stack0, align 4
  store void* null, void** %stack0, align 8
  %5 = bitcast i64* %_stack0 to void*
  store void* %5, void** %stack0, align 8
  %6 = load void*, void** %stack0, align 8
  call void @basic_new_stack(void* %6)
  %7 = load void*, void** %stack0, align 8
  call void @basic_const_pi(void* %7)
  %const_list = alloca %list, align 8
  %8 = call i8* (i32, ...) @_lfortran_malloc(i32 8)
  %9 = bitcast i8* %8 to void**
  %10 = getelementptr %list, %list* %const_list, i32 0, i32 2
  store void** %9, void*** %10, align 8
  %11 = getelementptr %list, %list* %const_list, i32 0, i32 0
  store i32 1, i32* %11, align 4
  %12 = getelementptr %list, %list* %const_list, i32 0, i32 1
  store i32 1, i32* %12, align 4
  %13 = getelementptr %list, %list* %const_list, i32 0, i32 2
  %14 = load void**, void*** %13, align 8
  %15 = getelementptr inbounds void*, void** %14, i32 0
  store void** %stack0, void** %15, align 8
  %16 = getelementptr %list, %list* %const_list, i32 0, i32 0
  %17 = load i32, i32* %16, align 4
  %18 = getelementptr %list, %list* %const_list, i32 0, i32 1
  %19 = load i32, i32* %18, align 4
  %20 = getelementptr %list, %list* %l1, i32 0, i32 0
  %21 = getelementptr %list, %list* %l1, i32 0, i32 1
  store i32 %17, i32* %20, align 4
  store i32 %19, i32* %21, align 4
  %22 = getelementptr %list, %list* %const_list, i32 0, i32 2
  %23 = load void**, void*** %22, align 8
  %24 = mul i32 8, %19
  %25 = call i8* (i32, ...) @_lfortran_malloc(i32 %24)
  %26 = bitcast i8* %25 to void**
  %27 = bitcast void** %26 to i8*
  %28 = bitcast void** %23 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %27, i8* %28, i32 %24, i1 false)
  %29 = getelementptr %list, %list* %l1, i32 0, i32 2
  store void** %26, void*** %29, align 8
  %30 = load void*, void** %stack0, align 8
  call void @basic_free_stack(void* %30)
  br label %return

return:                                           ; preds = %.entry
  ret void
}

The error

code generation error: asr_to_llvm: module failed verification. Error:
Stored value type does not match pointer operand type!
  store void** %stack0, void** %15, align 8
 void*

anutosh491 · 2023-12-08T05:19:04Z

I tried some workarounds to support both but unfortunately both the cases contradict each other.

certik · 2024-01-03T18:19:39Z

The failure file has the following relevant ASR (after all passes):

                                           l1:
                                                (Variable
                                                    3
                                                    l1
                                                    []
                                                    Local
                                                    ()
                                                    ()
                                                    Default
                                                    (List
                                                        (CPtr)
                                                    )
                                                    ()
                                                    Source
                                                    Public
                                                    Required
                                                    .false.
                                                ),
...
                                   (=
                                        (Var 3 l1)
                                        (ListConstant
                                            [(Var 3 stack0)]
                                            (List
                                                (CPtr)
                                            )
                                        )
                                        ()
                                    )

We are assigning a list of CPtr. The ASR looks correct, but the generated code is incorrect.

A similar test with ints:

def test_extraction_of_elements():
    l1: list[i32] = [5]

test_extraction_of_elements()

Produces

                                           l1:
                                                (Variable
                                                    3
                                                    l1
                                                    []
                                                    Local
                                                    ()
                                                    ()
                                                    Default
                                                    (List
                                                        (Integer 4)
                                                    )
                                                    ()
                                                    Source
                                                    Public
                                                    Required
                                                    .false.
                                                )
...
                                    [(=
                                        (Var 3 l1)
                                        (ListConstant
                                            [(IntegerConstant 5 (Integer 4))]
                                            (List
                                                (Integer 4)
                                            )
                                        )
                                        ()
                                    )]

And the corresponding LLVM is:

define void @__module___main___test_extraction_of_elements() {
.entry:
  %l1 = alloca %list, align 8
  %0 = call i8* (i32, ...) @_lfortran_malloc(i32 4)
  %1 = bitcast i8* %0 to i32*
  %2 = getelementptr %list, %list* %l1, i32 0, i32 2
  store i32* %1, i32** %2, align 8
  %3 = getelementptr %list, %list* %l1, i32 0, i32 0
  store i32 0, i32* %3, align 4
  %4 = getelementptr %list, %list* %l1, i32 0, i32 1
  store i32 1, i32* %4, align 4
  %const_list = alloca %list, align 8
  %5 = call i8* (i32, ...) @_lfortran_malloc(i32 4)
  %6 = bitcast i8* %5 to i32*
  %7 = getelementptr %list, %list* %const_list, i32 0, i32 2
  store i32* %6, i32** %7, align 8
  %8 = getelementptr %list, %list* %const_list, i32 0, i32 0
  store i32 1, i32* %8, align 4
  %9 = getelementptr %list, %list* %const_list, i32 0, i32 1
  store i32 1, i32* %9, align 4
  %10 = getelementptr %list, %list* %const_list, i32 0, i32 2
  %11 = load i32*, i32** %10, align 8
  %12 = getelementptr inbounds i32, i32* %11, i32 0
  store i32 5, i32* %12, align 4
  %13 = getelementptr %list, %list* %const_list, i32 0, i32 0
  %14 = load i32, i32* %13, align 4
  %15 = getelementptr %list, %list* %const_list, i32 0, i32 1
  %16 = load i32, i32* %15, align 4
  %17 = getelementptr %list, %list* %l1, i32 0, i32 0
  %18 = getelementptr %list, %list* %l1, i32 0, i32 1
  store i32 %14, i32* %17, align 4
  store i32 %16, i32* %18, align 4
  %19 = getelementptr %list, %list* %const_list, i32 0, i32 2
  %20 = load i32*, i32** %19, align 8
  %21 = mul i32 4, %16
  %22 = call i8* (i32, ...) @_lfortran_malloc(i32 %21)
  %23 = bitcast i8* %22 to i32*
  %24 = bitcast i32* %23 to i8*
  %25 = bitcast i32* %20 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %24, i8* %25, i32 %21, i1 false)
  %26 = getelementptr %list, %list* %l1, i32 0, i32 2
  store i32* %23, i32** %26, align 8
  br label %return

return:                                           ; preds = %.entry
  ret void
}

And that works.

This is to be compared with CPtr:

  %25 = call i8* (i32, ...) @_lfortran_malloc(i32 %24)
  %26 = bitcast i8* %25 to void**
  %27 = bitcast void** %26 to i8*
  %28 = bitcast void** %23 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %27, i8* %28, i32 %24, i1 false)
  %29 = getelementptr %list, %list* %l1, i32 0, i32 2
  store void** %26, void*** %29, align 8
  %30 = load void*, void** %stack0, align 8
  call void @basic_free_stack(void* %30)
  br label %return

The line store void** %26, void*** %29, align 8 is wrong.

In master, it works:

  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %28, i8* %29, i32 %25, i1 false)
  %30 = getelementptr %list, %list* %l1, i32 0, i32 2
  store void** %27, void*** %30, align 8
  %31 = load void*, void** %stack0, align 8
  call void @basic_free_stack(void* %31)
  br label %return

certik · 2024-01-03T18:31:28Z

src/libasr/codegen/asr_to_llvm.cpp

+                ptr_loads = 0;
+            } else {
+                ptr_loads = 1;
+            }


I think the issue here is that if x.m_args[i] is an argument, then we want ptr_loads=0, but if it is pi (as in the other example), where it is a local (or global) variable, we want ptr_loads = 1.

We want to keep ptr_loads = 1; for all other types as well as for CPtr if it is a local/global variable. Only if it is CPtr and a function argument, then we want to use ptr_loads = 0;.

certik · 2024-01-03T18:36:19Z

Good:

  %9 = bitcast i8* %8 to void**
  %10 = getelementptr %list, %list* %const_list, i32 0, i32 2
  store void** %9, void*** %10, align 8

bad:

  %14 = load void**, void*** %13, align 8
  %15 = getelementptr inbounds void*, void** %14, i32 0
  store void** %stack0, void** %15, align 8

certik · 2024-01-03T18:48:15Z

src/libasr/codegen/asr_to_llvm.cpp

+                bool is_argument = false;
+                ASR::expr_t *var = x.m_args[i];
+                if (is_a<ASR::Var_t>(*var)) {
+                    ASR::symbol_t *var_sym = ASR::down_cast<ASR::Var_t>(var)
+                        ->m_v;
+                    if (is_a<ASR::Variable_t>(*var_sym)) {
+                        ASR::Variable_t *v = down_cast<ASR::Variable_t>(var_sym);
+                        if (v->m_intent == intent_local ||
+                            v->m_intent == intent_return_var ||
+                            !v->m_intent) {
+                            is_argument = false;
+                        } else {
+                            is_argument = true;
+                        }
+                    }
+                }


This whole code here should be refactored into a function that should be put into ASRUtils. We might already have a function for this.

certik · 2024-01-03T18:48:50Z

Fix the above comment, and add a test.

The solution in this commit should be simplified.

anutosh491 · 2024-01-07T12:50:19Z

I lack some experience working on shared branches but I was aware of how to use a branch (as in use the commits from it) and then build on top of it and finally raise a PR for the same. Hence I ended making #2453 which works as expected

anutosh491 · 2024-01-07T14:44:59Z

I now realized why even after pushing on the same branch, it created a new PR.
So basically earlier as written in the documentation (https://docs.lfortran.org/en/installation/) we end up using
git clone https://github.com/lfortran/lfortran.git and we don't end up cloning our fork, hence I had

(lf) anutosh491@spbhat68:~/lpython/lpython$ git remote -v
upstream  https://github.com/anutosh491/lpython.git (fetch)
upstream  https://github.com/anutosh491/lpython.git (push)
origin        https://github.com/lcompilers/lpython.git (fetch)
origin        https://github.com/lcompilers/lpython.git (push)

I realized long back but I made the switch just today to something like

(lf) anutosh491@spbhat68:~/lpython/lpython$ git remote -v
origin  https://github.com/anutosh491/lpython.git (fetch)
origin  https://github.com/anutosh491/lpython.git (push)
upstream        https://github.com/lcompilers/lpython.git (fetch)
upstream        https://github.com/lcompilers/lpython.git (push)

And though I can use my previous branches, github would end up creating a new branch out of then as I switched my origin. Luckily I don't have lot of open branches :)

certik · 2024-01-07T23:21:30Z

Just use origin https://github.com/lcompilers/lpython.git and anutosh https://github.com/anutosh491/lpython.git. Always push to anutosh, never to origin.

certik mentioned this pull request Dec 6, 2023

Error message not clear while dealing with symbolic lists #2439

Closed

certik commented Jan 3, 2024

View reviewed changes

certik assigned anutosh491 Jan 3, 2024

certik added 2 commits January 7, 2024 14:01

Do not load CPtr

73953d6

Only use ptr_loads = 0 if it is an argument

84d73e7

The solution in this commit should be simplified.

anutosh491 force-pushed the dont_load_cptr branch from ed35dd0 to 84d73e7 Compare January 7, 2024 08:31

anutosh491 mentioned this pull request Jan 7, 2024

Don't load CPtr if the variable under consideration is an argument #2453

Merged

anutosh491 closed this Jan 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not load CPtr #2440

Do not load CPtr #2440

certik commented Dec 6, 2023

anutosh491 commented Dec 8, 2023

anutosh491 commented Dec 8, 2023

anutosh491 commented Dec 8, 2023

certik commented Jan 3, 2024 •

edited

Loading

certik Jan 3, 2024

certik Jan 3, 2024

certik commented Jan 3, 2024

certik Jan 3, 2024

certik commented Jan 3, 2024

anutosh491 commented Jan 7, 2024

anutosh491 commented Jan 7, 2024

certik commented Jan 7, 2024

Do not load CPtr #2440

Do not load CPtr #2440

Conversation

certik commented Dec 6, 2023

anutosh491 commented Dec 8, 2023

anutosh491 commented Dec 8, 2023

anutosh491 commented Dec 8, 2023

certik commented Jan 3, 2024 • edited Loading

certik Jan 3, 2024

Choose a reason for hiding this comment

certik Jan 3, 2024

Choose a reason for hiding this comment

certik commented Jan 3, 2024

certik Jan 3, 2024

Choose a reason for hiding this comment

certik commented Jan 3, 2024

anutosh491 commented Jan 7, 2024

anutosh491 commented Jan 7, 2024

certik commented Jan 7, 2024

certik commented Jan 3, 2024 •

edited

Loading