Skip to content
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

Fixing Symbolic List assignment #2477

Merged
merged 5 commits into from
Feb 7, 2024
Merged

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Feb 4, 2024

Explaining the operations/changes that the ASR symbolic pass introduces

  1. The pass is intended to deal with list assignment. Now there can a couple cases possible when we think about it which are
i) l1 = [pi, sin(x)] # listconstant assigned to list var
ii) l1 = l2 # list var assigned to another list var
  1. Now as discussed here Seg Fault for case involving symbolic lists through llvm backend #2450 (comment) , case ii is always a deep copy and as observed works without any issues. Though this can surely be improved
    Rather than the simple assignment we could have something like
    i) Empty l1
    ii) Appening elements of l1 with basic_heap
    iii) basic_assign between elements of l1 & l2
    But that can be taken care of later

  2. The pass essentially deals with case i
    What happens is the following (Snippet 1 is transformed into Snippet 2)

// Snippet 1
    l1: list[S] = [x]
// Snippet2

    # Introduce a placeholder for list[CPtr] just like we have one for CPtr
    _l1: list[CPtr] = [x]
    l1: list[CPtr] = []

    # l1 = [x]
    i: i32 = 0
    for i in range(len(_l1)):
        tmp: CPtr = basic_new_heap()
        l1.append(tmp)
        basic_assign(l1[i], _l1[i])

@anutosh491 anutosh491 marked this pull request as draft February 4, 2024 09:11
@anutosh491
Copy link
Collaborator Author

I've added few comments for our understanding and I've also added the failing case raised here (#2450 (comment)) as a test.

@anutosh491 anutosh491 marked this pull request as ready for review February 4, 2024 11:31
@certik certik changed the title [WIP] Fixing Symbolic List assignment Fixing Symbolic List assignment Feb 5, 2024
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.

I thin this is good. Thanks for fixing this! I think you can merge it.

@anutosh491
Copy link
Collaborator Author

Thanks for the review. I think we can go forward with this. If we are looking to customize the symbolic pass even more (if we come across any improvement maybe) let's address it through subsequent PRs.

@anutosh491 anutosh491 enabled auto-merge (squash) February 6, 2024 08:14
@anutosh491
Copy link
Collaborator Author

anutosh491 commented Feb 6, 2024

cc @certik I am not sure if I have permissions to merge something untill all requirements are met (I can enable auto-merge squash or rabase when all requirements are met) .
Now that the macos build doesn't pass, I am not sure I can get this in.

@certik certik merged commit a6b9256 into lcompilers:main Feb 7, 2024
12 of 13 checks passed
@anutosh491 anutosh491 deleted the fix_symbolic_list branch May 28, 2024 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants