-
Notifications
You must be signed in to change notification settings - Fork 170
Initial support for global Lists #1384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Here I'm trying to move all the symbols in the global to the x: list[i32] = [1,2,3,4]
print(x[0]) Here is the current ASR output: (TranslationUnit
(SymbolTable
1
{
_lpython_main_program:
(Function
(SymbolTable
3
{
x:
(Variable
3
x
[]
Local
(ListConstant
[(IntegerConstant 1 (Integer 4 []))
(IntegerConstant 2 (Integer 4 []))
(IntegerConstant 3 (Integer 4 []))
(IntegerConstant 4 (Integer 4 []))]
(List
(Integer 4 [])
)
)
()
Default
(List
(Integer 4 [])
)
Source
Public
Required
.false.
)
})
_lpython_main_program
[]
[]
[(Print
()
[(ListItem
(Var 3 x)
(IntegerConstant 0 (Integer 4 []))
(Integer 4 [])
()
)]
()
()
)]
()
Source
Public
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
),
main_program:
(Program
(SymbolTable
2
{
})
main_program
[]
[(SubroutineCall
1 _lpython_main_program
()
[]
()
)]
)
})
[]
) Am I doing this correct? |
Something like this. I think for global statements we have a pass. For global variables it gets tricky, if other functions access them. Not sure what the best solution there is. |
+1, This seems to be tricky to handle. @certik @czgdp1807 @Smit-create |
We might need to discuss this over video. I think using TranslationUnit symbol table is fine. I think LLVM supports global variables. But we need to figure out all the details and dependencies. |
Here is an example to consider: x: list[i32] = [1,2,3,4]
def f():
print(x[1])
print(x[0])
f() The first step is to turn all global statements and expressions into a function. We already have an existing x: list[i32] = [1,2,3,4]
def f():
print(x[1])
def _global_statements():
print(x[0])
f() There is one issue: it's not clear from ASR that the new generated function The major issue is how to implement the above global variables. A full robust solution requires a solid support for closures and lambda functions with capture and we are working on that. Until then, let's create a temporary solution. The temporary solution will assume that the global variables are only accessed from the global statements, and not from any other function. That is a very common case and for this case, all we need to do is to improve the |
src/libasr/pass/global_stmts.cpp
Outdated
unit.m_global_scope->move_symbols_from_global_scope(fn_scope, symbols); | ||
for (auto &a: symbols) { | ||
unit.m_global_scope->erase_symbol(a); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not do this every time. Either as an option (via an argument), or even better, refactor this into a function and let the frontend call it. Add verify() in this refactored function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add this function into global_stmts.h
, so there will be two separate functions: one moves the global statements/expressions, the other moves the global variables.
This function is just a temporary solution, as later once we have solid closure support, we don't need to call this, as the global variables would be handled like any other parent scope variables.
f9d7013
to
12de970
Compare
The following example seems to work:
But, I got some doubts: @czgdp1807 @certik thoughts? |
9090781
to
47a1d0c
Compare
We need to discuss more on this for further implementations. I have a doubt, can we have an External symbol in TranslationUnit sym_table? I think the ASR seems correct, but the LLVM fails: x: list[i32]
x = [1,2,3]
print(x, x[0])
def main0():
print(x[0]) ASR: (TranslationUnit
(SymbolTable
1
{
_lpython_main_program:
(Function
(SymbolTable
4
{
x:
(Variable
4
x
[]
Local
()
()
Default
(List
(Integer 4 [])
)
Source
Public
Required
.false.
)
})
_lpython_main_program
[main0]
[]
[(=
(Var 1 x)
(ListConstant
[(IntegerConstant 1 (Integer 4 []))
(IntegerConstant 2 (Integer 4 []))]
(List
(Integer 4 [])
)
)
()
)
(Print
()
[(ListItem
(Var 1 x)
(IntegerConstant 1 (Integer 4 []))
(Integer 4 [])
()
)]
()
()
)
(SubroutineCall
1 main0
()
[]
()
)]
()
Source
Public
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
),
main0:
(Function
(SymbolTable
2
{
})
main0
[]
[]
[(Print
()
[(ListItem
(Var 1 x)
(IntegerConstant 1 (Integer 4 []))
(Integer 4 [])
()
)]
()
()
)]
()
Source
Public
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
),
main_program:
(Program
(SymbolTable
3
{
})
main_program
[]
[(SubroutineCall
1 _lpython_main_program
()
[]
()
)]
),
x:
(ExternalSymbol
1
x
4 x
_lpython_main_program
[]
x
Public
)
})
_lpython_main_program
[]
) LLVM: code generation error: asr_to_llvm: module failed verification. Error:
Instruction does not dominate all uses!
%x = alloca %list, align 8
%0 = getelementptr %list, %list* %x, i32 0, i32 0
Instruction does not dominate all uses!
%x = alloca %list, align 8
%4 = getelementptr %list, %list* %x, i32 0, i32 2 |
Related: lfortran/lfortran#1059 |
Let's create two passes:
The second bullet point seems like a clean approach: each module in ASR is self contained, it cannot access anything from global scope (=translation unit's symbol table), it accesses everything via ExternalSymbol. The main program currently can call functions from global scope. Finally, the only place that can call global variables in global scope (translation unit) are other functions from global scope (translation unit). So if we promote translation unit to a module, the new translation unit will only have a main program and other modules, and only the main program has to be modified to use ExternalSymbol from this new module. |
47a1d0c
to
3c8d70c
Compare
x: list[i32]
x = [1,2,3]
print(x, x[0])
def main0():
print(x[0]) ASR output with current changes: (TranslationUnit
(SymbolTable
1
{
_global_symbols:
(Module
(SymbolTable
5
{
_lpython_main_program:
(Function
(SymbolTable
4
{
x:
(ExternalSymbol
4
x
5 x
_global_symbols
[]
x
Public
)
})
_lpython_main_program
[]
[]
[(=
(Var 4 x)
(ListConstant
[(IntegerConstant 1 (Integer 4 []))
(IntegerConstant 2 (Integer 4 []))
(IntegerConstant 3 (Integer 4 []))]
(List
(Integer 4 [])
)
)
()
)
(Print
()
[(Var 5 x)
(ListItem
(Var 5 x)
(IntegerConstant 0 (Integer 4 []))
(Integer 4 [])
()
)]
()
()
)]
()
Source
Public
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
),
main0:
(Function
(SymbolTable
2
{
x:
(ExternalSymbol
2
x
5 x
_global_symbols
[]
x
Public
)
})
main0
[]
[]
[(Print
()
[(ListItem
(Var 2 x)
(IntegerConstant 0 (Integer 4 []))
(Integer 4 [])
()
)]
()
()
)]
()
Source
Public
Implementation
()
.false.
.false.
.false.
.false.
.false.
[]
[]
.false.
),
x:
(Variable
5
x
[]
Local
()
()
Default
(List
(Integer 4 [])
)
Source
Public
Required
.false.
)
})
_global_symbols
[]
.false.
.false.
),
main_program:
(Program
(SymbolTable
3
{
_lpython_main_program:
(ExternalSymbol
3
_lpython_main_program
5 _lpython_main_program
_global_symbols
[]
_lpython_main_program
Public
)
})
main_program
[]
[(SubroutineCall
3 _lpython_main_program
()
[]
()
)]
)
})
[]
) |
After everything works (List in the LLVM), then I will create a separate pass for |
lpython/src/libasr/codegen/asr_to_llvm.cpp Line 2855 in 2c5034a
This |
3c8d70c
to
104f5f9
Compare
104f5f9
to
e530245
Compare
In here I would simply focus on creating a pass that transform the global statements and variables from TranslationUnit into a module. And you can create integration_test, and test it using ASR tests in Then in a separate PR let's test that module variables (of all types, including lists) work. |
3635e32
to
468379f
Compare
I followed the first approach from here: #1491 (comment) @czgdp1807 What do you think about this? |
Looks good so far. You can add a test, get everything passing. Then you can figure out how to change |
468379f
to
322f9c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
73d75d1
to
935c560
Compare
Should I try this in this PR? @certik |
935c560
to
2ec8413
Compare
Actually, it was easy to figure out. |
2264b79
to
06e58ed
Compare
There are some test failures. |
06e58ed
to
0a6e983
Compare
src/libasr/codegen/asr_to_llvm.cpp
Outdated
@@ -2296,6 +2297,14 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor> | |||
} | |||
} | |||
llvm_symtab[h] = ptr; | |||
} else if (x.m_type->type == ASR::ttypeType::List) { | |||
llvm::StructType* list_type = (llvm::StructType *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this C style cast needed? I would recommend to go for,
llvm::StructType* list_type = (llvm::StructType *) | |
llvm::StructType* list_type = static_cast<llvm::StructType*>( |
src/libasr/codegen/asr_to_llvm.cpp
Outdated
@@ -2296,6 +2297,14 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor> | |||
} | |||
} | |||
llvm_symtab[h] = ptr; | |||
} else if (x.m_type->type == ASR::ttypeType::List) { | |||
llvm::StructType* list_type = (llvm::StructType *) | |||
get_type_from_ttype_t_util(x.m_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_type_from_ttype_t_util(x.m_type); | |
get_type_from_ttype_t_util(x.m_type)); |
|
||
void visit_Assert(const AST::Assert_t &/*x*/) { | ||
// We skip this in the SymbolTable visitor, but visit it in the BodyVisitor | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should generate this for all the statement types. In a different PR though after this goes in.
|
0a6e983
to
f4ced93
Compare
f4ced93
to
f86901d
Compare
The thing is I switched from the ; ModuleID = 'LFortran'
source_filename = "LFortran"
%list = type { i32, i32, i32* }
@i = global i32 0
@x = global %list zeroinitializer <-----
... |
llvm::StructType* list_type = static_cast<llvm::StructType*>( | ||
get_type_from_ttype_t_util(x.m_type)); | ||
llvm::Constant *ptr = module->getOrInsertGlobal(x.m_name, list_type); | ||
module->getNamedGlobal(x.m_name)->setInitializer( | ||
llvm::ConstantStruct::get(list_type, | ||
llvm::Constant::getNullValue(list_type))); | ||
llvm_symtab[h] = ptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you can figure out how to change void* into "struct" as needed.
#1384 (comment)
Sorry, I think I understood this wrong!
@czgdp1807 is the approach used here correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using global variable is better. It stays in scope and is independent of all LLVM functions. I checked the generated LLVM code and it looks fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
@u = global i64 0 | ||
@x = global i32 0 | ||
@y = global i16 0 | ||
@z = global i8 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would keep these initializations here. This is relatively minor, so we can do it in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue for this at #1582.
Related: #1376