-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[flang][OpenACC] generate Destroy region to free memory of private and firstprivate if needed #162702
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
Conversation
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) ChangesThis patch extends the MappableTypeInterface to:
Question: I followed the Destroy design from OpenACC.td and also passed the original value. I am wondering if that it is needed (happy to keep it if you want it to be very generic). Full diff: https://github.com/llvm/llvm-project/pull/162702.diff 5 Files Affected:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you.
This issue was previously discussed here, and my reasoning is based on the principles of consistency, generality, and flexibility:
Moreover, the additional argument will be effectively a no-op after recipe materialization if the dialect-specific recipe implementation does not require it, thus incurring no cost. However, should we need to incorporate this in the future, it would involve considerable rework and IR test modifications. Since we are revisiting this design decision, I would like to either get buy-in or agree to change it and remove the extra argument now. @erichkeane @clementval |
Sounds reasonable. I do not see an issue keeping it, I was just wondering the rational. Thanks for the link and details! |
It doesn't matter to me. It is simply an unused argument so far for me, and I don't see myself using it again? If it were to disappear I'd need notice (or a verifier would be preferential!), but otherwise I have no problem with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! :) Thank you!
flang/include/flang/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.h
Outdated
Show resolved
Hide resolved
flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
Outdated
Show resolved
Hide resolved
flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
Outdated
Show resolved
Hide resolved
flang/lib/Optimizer/OpenACC/Support/FIROpenACCTypeInterfaces.cpp
Outdated
Show resolved
Hide resolved
I was more to remove it in the previous PR but I'm fine with either choice. |
…d firstprivate if needed (llvm#162702) This patch extends the MappableTypeInterface to: - allow genPrivateInit to indicate that a Destroy region will be needed. - add genPrivateDestroy to generate the destruction code - Implement both interfaces in FIR - Update OpenACC lowering to generate the Destroy region when needed using those interfaces.
This patch extends the MappableTypeInterface to:
Question: I followed the Destroy design from OpenACC.td and also passed the original value. I am wondering if that it is needed (happy to keep it if you want it to be very generic).