Skip to content

Support passing dict as return value of functions #1659

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

Merged
merged 11 commits into from
Apr 8, 2023

Conversation

harshsingh-24
Copy link
Contributor

@harshsingh-24 harshsingh-24 commented Apr 3, 2023

Adds to #983 by giving full support of dictionary return type in LLVM and C backend respectively.

@harshsingh-24 harshsingh-24 marked this pull request as draft April 3, 2023 06:49
@harshsingh-24
Copy link
Contributor Author

harshsingh-24 commented Apr 4, 2023

Hey @certik @Thirumalai-Shaktivel . Can you please help me out a little with this implementation? I am facing issues. If you can point me out a little to some portions. After doing this, it basically says AssertFailed: src->getType() == dest->getType() when deep_copy method of dict is being called.

Test case -

def fun() -> dict[i32, i32]:
    return {2: 4, 3: 6}

num : dict[i32, i32]
num = fun()
print(num[2])

Also, I have witnessed that while deep copying - the value dict and target dict are as follows -

VALUE DICT
 %0 = call %dict @__module__global_symbols_fun()
TARGET DICT
@num = global %dict zeroinitializer

I feel that for it should be something of %0 = alloca % dict, align 8 or similar to that? What do you think?

@czgdp1807
Copy link
Collaborator

czgdp1807 commented Apr 4, 2023

I tried your branch by commenting out the last two lines in your example,

from lpython import i32

def fun() -> dict[i32, i32]:
    return {2: 4, 3: 6}

num : dict[i32, i32]
# num = fun()
# print(num[2])

LLVM code,

; ModuleID = 'LFortran'
source_filename = "LFortran"

%dict = type { i32, %list, %list, i8* }
%list = type { i32, i32, i32* }

@num = global %dict zeroinitializer

define %dict @fun() {
.entry:
  %_lpython_return_variable = alloca %dict, align 8
  %const_dict = alloca %dict, align 8
  %0 = getelementptr %dict, %dict* %const_dict, i32 0, i32 0
  store i32 0, i32* %0, align 4
.
.
.

You can see fun is returning a dict (non-pointer) where as @num is a pointer that's why the assert fails. Now how do we fix this? In a way similar how we do it for lists (check integration_tests/test_list_01.py and see the LLVM code generated for that test, same pattern can be applied to dicts as well).

P.S. Also read #955 (comment)

@harshsingh-24
Copy link
Contributor Author

harshsingh-24 commented Apr 5, 2023

@czgdp1807 I added ASR::is_a<ASR::Dict_t>(*asr_type) to the check static inline bool is_llvm_struct(ASR::ttype_t* asr_type) in llvm_utils.h to incorporate Dict return type using the above logic as suggested by you.

@harshsingh-24
Copy link
Contributor Author

Can you please check @Thirumalai-Shaktivel and @czgdp1807 as to why the build is failing on C Backend?

@czgdp1807
Copy link
Collaborator

code generation error: Return type not supported in function 'fill_rollnumber2cpi'

You need to support dict as a return type in C backend as well (concerned files asr_to_c.cpp, asr_to_c_cpp.h and asr_to_cpp.h).

@harshsingh-24
Copy link
Contributor Author

Can you please review ? @Thirumalai-Shaktivel @czgdp1807 @certik

@harshsingh-24
Copy link
Contributor Author

Can you review? @czgdp1807 @certik . CI tests have passed.

@harshsingh-24 harshsingh-24 requested a review from certik April 6, 2023 08:32
@harshsingh-24 harshsingh-24 marked this pull request as ready for review April 6, 2023 13:23
@czgdp1807 czgdp1807 marked this pull request as draft April 7, 2023 11:02
@harshsingh-24 harshsingh-24 requested a review from czgdp1807 April 7, 2023 17:37
@harshsingh-24 harshsingh-24 marked this pull request as ready for review April 7, 2023 17:37
Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I would like @czgdp1807 to also review. The history needs polishing or squashing. I rebased on top of the latest master.

@czgdp1807 czgdp1807 enabled auto-merge (squash) April 8, 2023 06:27
@czgdp1807 czgdp1807 merged commit 2d25371 into lcompilers:main Apr 8, 2023
@harshsingh-24 harshsingh-24 deleted the dict-funcs branch April 8, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants