Conversation
|
This seems to be the correct fix for the concerned program But fails for |
|
Consider the following 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 The error |
|
I tried some workarounds to support both but unfortunately both the cases contradict each other. |
|
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 %returnThe line 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 |
| ptr_loads = 0; | ||
| } else { | ||
| ptr_loads = 1; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;.
|
Good: bad: |
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This whole code here should be refactored into a function that should be put into ASRUtils. We might already have a function for this.
|
Fix the above comment, and add a test. |
The solution in this commit should be simplified.
anutosh491
force-pushed
the
dont_load_cptr
branch
from
ed35dd0 to
84d73e7
Compare
|
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 |
|
I now realized why even after pushing on the same branch, it created a new PR. I realized long back but I made the switch just today to something like 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 :) |
|
Just use |
2 participants
No description provided.