Add String to List cast in LLVM #1098

Smit-create · 2022-09-07T14:41:06Z

No description provided.

src/libasr/codegen/asr_to_llvm.cpp

Thirumalai-Shaktivel · 2022-09-08T08:45:54Z

Applying this diff, made things to work, but the ouput

diff --git a/src/libasr/codegen/asr_to_llvm.cpp b/src/libasr/codegen/asr_to_llvm.cpp
index be298fb24..9a5ea3488 100644
--- a/src/libasr/codegen/asr_to_llvm.cpp
+++ b/src/libasr/codegen/asr_to_llvm.cpp
@@ -4773,19 +4773,17 @@ public:
                 break;
             }
             case (ASR::cast_kindType::CharacterToList) : {
-                this->visit_expr_wrapper(x.m_arg, true);
                 llvm::Value *str = CreateLoad(tmp);
                 llvm::AllocaInst *parg = builder->CreateAlloca(character_type, nullptr);
-                builder->CreateStore(str, parg);
+                LLVM::CreateStore(*builder, str, parg);
                 llvm::Value *str_len = lfortran_str_len(parg);
+
                 llvm::Type *type = getIntType(4);
-                llvm::Value *ptr = module->getOrInsertGlobal("_character_to_list_itr",
-                    type);
-                ptr = builder->CreateLoad(ptr);
-                module->getNamedGlobal("_character_to_list_itr")->setInitializer(
-                        llvm::ConstantInt::get(context,
-                            llvm::APInt(32, 0)));
+                llvm::AllocaInst *ptr = builder->CreateAlloca(type, nullptr);
                 llvm::Value* i32_one = llvm::ConstantInt::get(context, llvm::APInt(32, 1));
+                llvm::Value* i32_zero = llvm::ConstantInt::get(context, llvm::APInt(32, 0));
+                LLVM::CreateStore(*builder, i32_zero, ptr);
 
                 ASR::List_t* list_type = ASR::down_cast<ASR::List_t>(x.m_type);
                 bool is_array_type_local = false, is_malloc_array_type_local = false;
@@ -4815,14 +4813,16 @@ public:
 
                 // head
                 start_new_block(loophead);
-                llvm::Value *cond = builder->CreateICmpSLT(ptr, str_len);
+                llvm::Value *cond = builder->CreateICmpSLT(LLVM::CreateLoad(*builder, ptr), str_len);
                 builder->CreateCondBr(cond, loopbody, loopend);
 
                 // body
                 start_new_block(loopbody);
-                llvm::Value* item = lfortran_str_copy(str, ptr, ptr);
+                llvm::Value* item = lfortran_str_copy(str, LLVM::CreateLoad(*builder, ptr), LLVM::CreateLoad(*builder, ptr));
                 list_api->append(const_list, item, list_type->m_type, *module);
-                ptr = builder->CreateAdd(ptr, i32_one);
+                LLVM::CreateStore(*builder, builder->CreateAdd(LLVM::CreateLoad(*builder, ptr), i32_one), ptr);
 
                 builder->CreateBr(loophead);

def f():
    x: list[str]
    s: str
    s = "lpython"
    x = list(s)
    i:i32
    for i in range(len(x)):
        print(x[i], end=", ")
    print()

f()

$ lpython examples/expr2.py && ./a.out
n, l, p, y, t, h, o,

Thirumalai-Shaktivel · 2022-09-08T09:00:23Z

I figured it out, the string index starts from 1 to len(str). But I used 0 to len(str)
The following diff works, @Smit-create. Any doubts please ping me.

diff --git a/src/libasr/codegen/asr_to_llvm.cpp b/src/libasr/codegen/asr_to_llvm.cpp
index be298fb24..9989e4b07 100644
--- a/src/libasr/codegen/asr_to_llvm.cpp
+++ b/src/libasr/codegen/asr_to_llvm.cpp
@@ -4773,20 +4773,18 @@ public:
                 break;
             }
             case (ASR::cast_kindType::CharacterToList) : {
-                this->visit_expr_wrapper(x.m_arg, true);
                 llvm::Value *str = CreateLoad(tmp);
                 llvm::AllocaInst *parg = builder->CreateAlloca(character_type, nullptr);
-                builder->CreateStore(str, parg);
+                LLVM::CreateStore(*builder, str, parg);
                 llvm::Value *str_len = lfortran_str_len(parg);
+
                 llvm::Type *type = getIntType(4);
-                llvm::Value *ptr = module->getOrInsertGlobal("_character_to_list_itr",
-                    type);
-                ptr = builder->CreateLoad(ptr);
-                module->getNamedGlobal("_character_to_list_itr")->setInitializer(
-                        llvm::ConstantInt::get(context,
-                            llvm::APInt(32, 0)));
+                llvm::AllocaInst *ptr = builder->CreateAlloca(type, nullptr);
                 llvm::Value* i32_one = llvm::ConstantInt::get(context, llvm::APInt(32, 1));
+                LLVM::CreateStore(*builder, i32_one, ptr);
 
+                // Empty List Initialization
                 ASR::List_t* list_type = ASR::down_cast<ASR::List_t>(x.m_type);
                 bool is_array_type_local = false, is_malloc_array_type_local = false;
                 bool is_list_local = false;
@@ -4805,8 +4803,8 @@ public:
                 uint64_t ptr_loads_copy = ptr_loads;
                 ptr_loads = 1;
 
+                // Looping statement
                 dict_api->set_iterators();
-
                 llvm::BasicBlock *loophead = llvm::BasicBlock::Create(context, "loop.head");
                 llvm::BasicBlock *loopbody = llvm::BasicBlock::Create(context, "loop.body");
                 llvm::BasicBlock *loopend = llvm::BasicBlock::Create(context, "loop.end");
@@ -4815,15 +4813,16 @@ public:
 
                 // head
                 start_new_block(loophead);
-                llvm::Value *cond = builder->CreateICmpSLT(ptr, str_len);
+                llvm::Value *cond = builder->CreateICmpSLE(LLVM::CreateLoad(*builder, ptr), str_len);
                 builder->CreateCondBr(cond, loopbody, loopend);
 
                 // body
                 start_new_block(loopbody);
-                llvm::Value* item = lfortran_str_copy(str, ptr, ptr);
-                list_api->append(const_list, item, list_type->m_type, *module);
-                ptr = builder->CreateAdd(ptr, i32_one);
-
+                {
+                    llvm::Value* item = lfortran_str_copy(str, LLVM::CreateLoad(*builder, ptr), LLVM::CreateLoad(*builder, ptr));
+                    list_api->append(const_list, item, list_type->m_type, *module);
+                    LLVM::CreateStore(*builder, builder->CreateAdd(LLVM::CreateLoad(*builder, ptr), i32_one), ptr);
+                }
                 builder->CreateBr(loophead);
 
                 // end

Smit-create · 2022-09-08T10:10:48Z

Thanks, that worked.

Thirumalai-Shaktivel

LGTM!

integration_tests/str_to_list_cast.py

czgdp1807 · 2022-09-14T04:58:33Z

Correct me if I am wrong, so string to list cast can be implemented as follows at the Python's language level,

l: list[str] = []
for char in string:
    l.append(char)

If so then I think this cast should be handled at ASR pass level so that it can be shared across all the backends at once.

Smit-create · 2022-09-18T09:07:54Z

Correct me if I am wrong, so string to list cast can be implemented as follows at the Python's language level,
l: list[str] = []
for char in string:
    l.append(char)
If so then I think this cast should be handled at ASR pass level so that it can be shared across all the backends at once.

I tried to implement this, but I have a question now how we will make it to return l?

certik · 2022-09-21T00:52:58Z

I think @czgdp1807 is probably right. So effectively this:

l: list[str] = list(string)

is equivalent to this:

l: list[str] = []
for char in string:
    l.append(char)

But in terms of performance, we might (later) do something like:

l: list[str]
l.reserve(len(string))
for i in range(len(string)):
    l[i] = string[i]

If it is faster but we'll get to these optimizations later. Probably our (future) optimizer might be able to do such optimizations also.

How to return l?

Here is one approach: turn l: list[str] = list(string) into l: list[str] = _lcompilers_string_to_list(string) and implement a function (even by hand in our runtime library):

def _lcompilers_string_to_list(string: str) -> list[str]:
    l: list[str] = []
    for char in string:
        l.append(char)
    return l

And make the frontend AST->ASR insert it. That's all that is needed I think. (So you don't even need to create a new pass.)

…ke_call_helper

Smit-create · 2022-09-21T09:03:48Z

I have updated the string to list cast in the ASR frontend. Please review.

czgdp1807 · 2022-09-21T09:05:13Z

Here is one approach: turn l: list[str] = list(string) into l: list[str] = _lcompilers_string_to_list(string) and implement a function (even by hand in our runtime library):
def _lcompilers_string_to_list(string: str) -> list[str]:
    l: list[str] = []
    for char in string:
        l.append(char)
    return l
And make the frontend AST->ASR insert it. That's all that is needed I think. (So you don't even need to create a new pass.)

I would prefer this approach over others.

certik · 2022-09-21T12:10:56Z

src/lpython/semantics/python_ast_to_asr.cpp

+                Vec<ASR::expr_t*> list;
+                list.reserve(al, 1);
+                return ASR::make_ListConstant_t(al, loc, list.p,
+                    list.size(), list_type);


Is this creating an empty list constant?

certik · 2022-09-21T12:12:12Z

src/lpython/semantics/python_comptime_eval.h

+                "' object conversion to List is not implemented ",
+                arg->base.loc);
+        }
+    }


This is handling compile time list("abcd") stuff? If so, we need to add tests for that. The above tests seem to test runtime conversion, which is not implemented in this PR yet (I think).

Compile time also works, those tests are present in the same file from line 1-10.

certik · 2022-09-21T12:14:28Z

src/runtime/lpython_builtin.py

+    i: i32
+    for i in range(len(s)):
+        l.append(s[i])
+    return l


Ah I see, it's doing the runtime as well. So we should test both compile time and runtime.

For runtime, should we rename this function to _lcompilers_str_to_list, just to make it clear that this is an internal function?

CC @czgdp1807.

I think list is more approriate because it is a builtin function or intrinsic in python much like ord, hex, bin etc. and we can handle it the same way.

certik · 2022-09-21T12:15:28Z

I would prefer this approach over others.

I think it is doing that approach, but there are a few things to polish, see above.

certik

I think this is good to go in. Everything is needed in this PR.

Review fixed

Smit-create marked this pull request as draft September 7, 2022 14:41

Smit-create force-pushed the strtolist2 branch from c5244c6 to 598d183 Compare September 8, 2022 06:10

Smit-create commented Sep 8, 2022

View reviewed changes

src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved

Smit-create commented Sep 8, 2022

View reviewed changes

src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved

Thirumalai-Shaktivel reviewed Sep 8, 2022

View reviewed changes

src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved

Thirumalai-Shaktivel reviewed Sep 8, 2022

View reviewed changes

src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved

Smit-create force-pushed the strtolist2 branch from 598d183 to 58abda9 Compare September 8, 2022 10:10

Smit-create marked this pull request as ready for review September 8, 2022 10:10

Smit-create added the llvm LLVM related changes label Sep 8, 2022

Smit-create requested review from Thirumalai-Shaktivel and czgdp1807 September 8, 2022 10:13

Thirumalai-Shaktivel approved these changes Sep 8, 2022

View reviewed changes

czgdp1807 previously requested changes Sep 8, 2022

View reviewed changes

integration_tests/str_to_list_cast.py Show resolved Hide resolved

Smit-create force-pushed the strtolist2 branch from 58abda9 to 9c760fb Compare September 8, 2022 11:33

Smit-create requested a review from czgdp1807 September 10, 2022 06:55

Smit-create force-pushed the strtolist2 branch from 9c760fb to 3d5ec07 Compare September 17, 2022 12:06

Smit-create force-pushed the strtolist2 branch from 3d5ec07 to 16ede28 Compare September 21, 2022 08:57

Smit-create added 4 commits September 21, 2022 14:30

Remove unused functions and args

cbd703b

Handle a case of empty list where we need to type of ann_assign in ma…

b9a44a9

…ke_call_helper

Add intrinsic list for conversion of string to list

a36df11

Add and update tests

ece6c73

Smit-create force-pushed the strtolist2 branch from 16ede28 to ece6c73 Compare September 21, 2022 09:02

certik reviewed Sep 21, 2022

View reviewed changes

certik mentioned this pull request Sep 21, 2022

--new-parser fails #1132

Closed

Fix a compiler warning

b510150

certik approved these changes Sep 21, 2022

View reviewed changes

Smit-create enabled auto-merge September 21, 2022 16:52

Smit-create merged commit 9646432 into lcompilers:main Sep 21, 2022

Smit-create deleted the strtolist2 branch September 21, 2022 17:24

Add String to List cast in LLVM #1098

Add String to List cast in LLVM #1098

Uh oh!

Conversation

Smit-create commented Sep 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Sep 8, 2022

Uh oh!

Thirumalai-Shaktivel commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Smit-create commented Sep 8, 2022

Uh oh!

Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

czgdp1807 commented Sep 14, 2022

Uh oh!

Smit-create commented Sep 18, 2022

Uh oh!

certik commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Smit-create commented Sep 21, 2022

Uh oh!

czgdp1807 commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Smit-create Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

certik Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Smit-create Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Smit-create Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

certik commented Sep 21, 2022

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thirumalai-Shaktivel commented Sep 8, 2022 •

edited

Loading

certik commented Sep 21, 2022 •

edited

Loading

czgdp1807 commented Sep 21, 2022 •

edited

Loading

Smit-create Sep 21, 2022 •

edited

Loading