-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[flang][OpenMP] Fix for #86393 #87452
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
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.
Thank you for fixing this. The changes to generated IR look good to me. Please could you add a test similar to flang/test/Lower/OpenMP/parallel-reduction-array.f90
which covers this case so that it does not get missed in the future.
(This shouldn't block the patch, but) I don't fully understand why skipping one level in the symbol table results in us using the correct result from the hlfir.declare. Please could you explain this a bit more for me?
Okay
Something has to do with: |
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Sourabh Singh Tomar (SouraVX) ChangesWhen checking symbol details:
|
// For Host associated symbols, use `SymbolBox` instead | ||
Fortran::lower::SymbolBox symBox = | ||
converter.lookupOneLevelUpSymbol(*symbol); |
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.
Why does looking one level up always work, even in cases where the symbol is not host associated (e.g. in the tests modified by this PR)?
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.
Okay so my understanding is that lookOneLevelUp will always get the host symbol from the function scope. Previously we were using the privatized version of the symbol from the OpenMP construct scope.
It isn't clear to me why the OpenMP construct scope maps the symbol to the second result of the hlfir.declare, meanwhile the function scope uses the first result of the declare (which then gives the right result here). I wonder if that is an OpenMP bug.
But as this is a crash seen in the wild, I do not want to hold up this PR further.
Thank you for fixing this.
I think |
As I understand it, this is what lookOneLevelUp is resulting in, which is what fixes the bug here (the |
When checking symbol details: