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.

Sign up for GitHub

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

Jump to bottom

Fix deallocate printer #1266

Merged
merged 74 commits into from
Apr 19, 2023
Merged

Fix deallocate printer #1266

merged 74 commits into from
Apr 19, 2023

Conversation

framdani
Copy link
Member

@framdani framdani commented Dec 20, 2022 •
edited by EmilyBourne
Loading

Update the printer of deallocate to free the array object allocated by one of the cuda's memory allocation APIs. Fixes pyccel#8

Functions added:

  • cuda_free_host : frees the memory returned by cudaMallocHost().
  • cuda_free : frees the array located on the device.
  • cuda_free_pointer : frees a pointer to a cuda array object.

@framdani framdani marked this pull request as draft December 20, 2022 10:58
@framdani framdani added the Language:Ccuda stuff related to ccuda code label Dec 20, 2022
@framdani framdani marked this pull request as ready for review December 22, 2022 12:37
@framdani framdani requested a review from bauom December 22, 2022 12:37
@framdani framdani requested a review from bauom December 23, 2022 11:01
bauom
bauom requested changes Dec 29, 2022
View reviewed changes
tests/internal/scripts/ccuda/deallocate.py Outdated Show resolved Hide resolved
@EmilyBourne
Copy link
Member

Codacy tests are failing

@EmilyBourne EmilyBourne requested a review from Pinkyboi December 30, 2022 11:21
@bauom
Copy link
Contributor

bauom commented Jan 5, 2023

@EmilyBourne is this CoverageChecker error intended and we should not create a variable like that.

@bauom
Copy link
Contributor

bauom commented Jan 5, 2023

@framdani i think in _print_Deallocate you can just return the Values because I don't see the need for dealloc_code Variable.

@EmilyBourne
Copy link
Member

@EmilyBourne is this CoverageChecker error intended and we should not create a variable like that.

Huh, I thought I'd pulled the fix for that bug. It's not intended. There's a file where I accidentally save the line instead of the line index. I'll fix it once I find somewhere I can get my computer out

@EmilyBourne
Copy link
Member

@EmilyBourne is this CoverageChecker error intended and we should not create a variable like that.

Huh, I thought I'd pulled the fix for that bug. It's not intended. There's a file where I accidentally save the line instead of the line index. I'll fix it once I find somewhere I can get my computer out

Ah sorry, wrong PR. The fix is in #1279. You can either pull the changes from there or wait until that PR is merged and update

EmilyBourne
EmilyBourne requested changes Jan 6, 2023
View reviewed changes
pyccel/codegen/printing/ccudacode.py Outdated Show resolved Hide resolved
pyccel/codegen/printing/ccudacode.py Outdated Show resolved Hide resolved
bauom
bauom reviewed Jan 9, 2023
View reviewed changes
Copy link
Contributor

@bauom bauom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

everything seems good to me now.
will merge in the correct order to fix the failing tests.

@EmilyBourne EmilyBourne mentioned this pull request Mar 17, 2023
Merged
@framdani framdani self-assigned this Mar 17, 2023
EmilyBourne
EmilyBourne requested changes Mar 21, 2023
View reviewed changes
tests/pyccel/scripts/test2.json Outdated Show resolved Hide resolved
pyccel/codegen/printing/ccudacode.py Show resolved Hide resolved
@EmilyBourne
Copy link
Member

Please could you merge cuda_main_temp into this branch. The diff is quite misleading at the moment

@EmilyBourne
Copy link
Member

EmilyBourne commented Mar 21, 2023 •
edited
Loading

Fixes pyccel-cuda#8 ?

@framdani
Copy link
Member Author

Fixes pyccel-cuda#8 ?

Yes this PR fixes pyccel-cuda#8

@framdani
Copy link
Member Author

Please could you merge cuda_main_temp into this branch. The diff is quite misleading at the moment

Fixed here 2a03c9c

@EmilyBourne EmilyBourne requested a review from bauom March 22, 2023 07:31
@EmilyBourne EmilyBourne linked an issue Mar 22, 2023 that may be closed by this pull request
fix Deallocate printer pyccel/pyccel-cuda#8
Open
bauom
bauom approved these changes Mar 22, 2023
View reviewed changes
pyccel/codegen/printing/ccudacode.py Show resolved Hide resolved
@framdani framdani added Ready_for_review Received at least one approval. Requires review from senior developer and removed needs_initial_review labels Mar 30, 2023
@bauom bauom merged commit 0bbad8f into cuda_main_temp Apr 19, 2023
@bauom bauom deleted the fix-1248 branch April 19, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbdelkhalekBAROUDI AbdelkhalekBAROUDI AbdelkhalekBAROUDI left review comments

@bauom bauom bauom approved these changes

@EmilyBourne EmilyBourne EmilyBourne approved these changes

Assignees

@framdani framdani

Labels
Language:Ccuda stuff related to ccuda code Ready_for_review Received at least one approval. Requires review from senior developer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix Deallocate printer
4 participants
@framdani @EmilyBourne @bauom @AbdelkhalekBAROUDI