Skip to content

Fix : Copy array descriptor info with associate expression#6618

Merged
gxyd merged 10 commits into
lfortran:mainfrom
assem2002:copy_dim_des_with_associate
Mar 15, 2025
Merged

Fix : Copy array descriptor info with associate expression#6618
gxyd merged 10 commits into
lfortran:mainfrom
assem2002:copy_dim_des_with_associate

Conversation

@assem2002

@assem2002 assem2002 commented Mar 14, 2025

Copy link
Copy Markdown
Contributor

Fixes #6611

  • Shallow copy data ptr
  • Deep copy dimension descriptor + offset value

This change led to :

  • Change on how we store null() in arrays (it used to set the whole descriptor to null and we fully depend on declared arrays to be ready for use)

@assem2002

assem2002 commented Mar 14, 2025

Copy link
Copy Markdown
Contributor Author

The change should've been very small. but how we relied on store with associate made us fell free to use null however we wanted (associate null or nullifying both lead to removing the whole array descriptor- and we heavily rely on the fact that any declared variable should have its whole struct set and ready to go.)

@assem2002

Copy link
Copy Markdown
Contributor Author

Mainly : testing CI

@certik

certik commented Mar 14, 2025

Copy link
Copy Markdown
Contributor

Thanks for fixing this. Yes, let's first figure out a solution that works, then we can clean it up. This is related to #6612 also.

@certik certik marked this pull request as draft March 15, 2025 01:11
- Shallow copy data ptr
- Deep copy dimension descriptor + offset value

This change led to :
- Change on how we store `null()` in arrays (it used to set the whole descriptor to null and we fully depend on declared arrays to be ready for use)
@assem2002 assem2002 force-pushed the copy_dim_des_with_associate branch from 23c27e6 to 41e6b8a Compare March 15, 2025 08:43
@assem2002 assem2002 marked this pull request as ready for review March 15, 2025 08:43
…descriptor` instead of setting the whole variable to null,

So we know for sure that we have the variable set with the descriptor and act upon that fact with other functions and visitors
Comment thread integration_tests/CMakeLists.txt Outdated
@assem2002 assem2002 force-pushed the copy_dim_des_with_associate branch from 5d90cd8 to 95d9859 Compare March 15, 2025 10:13
@assem2002

assem2002 commented Mar 15, 2025

Copy link
Copy Markdown
Contributor Author

Ready to review. maybe squash merge.
I tried to make the commits a bit descriptive.

@gxyd

gxyd commented Mar 15, 2025

Copy link
Copy Markdown
Contributor

Fixes #6611

I checked locally on my machine, that this PR allows us to now run validation with POT3D with LFortran's --fast optimizations enabled successfully.

And fixes both the MRE's reported in the linked issue.

@Pranavchiku

Copy link
Copy Markdown
Member

Woahh! super news.

@Pranavchiku Pranavchiku left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a quick review, this looks good to me!

@Pranavchiku

Copy link
Copy Markdown
Member

This is minor and can be done in subsequent PRs but let's add all the reported MREs at #6611 to integration tests.

@Pranavchiku Pranavchiku mentioned this pull request Mar 15, 2025
24 tasks

@gxyd gxyd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@certik

certik commented Mar 15, 2025

Copy link
Copy Markdown
Contributor

Great job @assem2002 !!!

The change is bigger and I didn't review every detail, and we can review the design at our next meeting, but I think it's likely the correct fix.

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.

MRE for pot3d --fast compilation

4 participants