-
-
Notifications
You must be signed in to change notification settings - Fork 207
Fix intrinsic function dependency on external symbol in dims #2384
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
1062701
to
2df6d02
Compare
The reference tests seem to be different on Mac and Ubuntu. |
if( ASR::is_a<ASR::ArrayPhysicalCast_t>(*(*current_expr)) ) { | ||
ASR::ArrayPhysicalCast_t* array_physical_cast_t = ASR::down_cast<ASR::ArrayPhysicalCast_t>(*current_expr); | ||
array_physical_cast_t->m_arg = current_expr_; | ||
} else { | ||
*current_expr = current_expr_; | ||
} |
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.
It might not be needed for LFortran but might be needed for LPython. Can you try checking with LPython for this removal?
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.
Or if there is any theoretical explanation that after removing this piece of code, things will still be the same then please provide that.
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.
It might not be needed for LFortran but might be needed for LPython. Can you try checking with LPython for this removal?
I extracted out the specific code and submitted PRs with this particular change #2387 at LFortran and lcompilers/lpython#2315 at LPython. For LPython I simply copied the whole file intrinsic_function.cpp
in LFortran and pasted it in LPython (I think LFortran has the newer version of the file).
I think if the tests passed at both, it would be good to merge.
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.
if there is any theoretical explanation that after removing this piece of code, things will still be the same then please provide that.
Yes, I shared the explanation at #2387
That's most probably because of the difference in the order in which clang and GCC process the function arguments. Might be some improvements needed in LLVM backend. You might have to look into this yourself until we meet the next time. |
2db3cb9
to
962302d
Compare
962302d
to
230625f
Compare
I updated the description of this issue. This PR is ready. |
This PR fixes the following issue: