-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fixes a host function called from host device function. #1338
Fixes a host function called from host device function. #1338
Conversation
…U cases where it should not. This silences many warnings about calling std::vector::operator[] when not appropriate.
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.
Thanks @BradWhitlock!
Note: I think this PR is related to #1196
Tag: @rhornung67
abort(); | ||
#else | ||
// Always return a value. | ||
return buf[pos]; | ||
#endif | ||
#endif | ||
#else | ||
// Always return a value. | ||
return buf[pos]; | ||
#endif |
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.
Is this change meant to be more explicit?
My read is that the previous behavior always called return buf[pos]
(regardless of whether AXOM_DEVICE_CODE
is defined) and the updated behavior does the same.
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.
I suppose I could have removed the comment " Always return a value". The problem for the CUDA/HIP branches of these functions was that an STLVectorIndirection was passed into the IndexedIndirection and the operator[] it ends up using is not a host-device function and never could be, resulting in a lot of compilation warnings on rzansel. This small change skips calling buf[pos] for the CUDA/HIP branches since they abort first anyway.
#else | ||
// Always return a value. | ||
return buf[pos]; | ||
#endif | ||
#endif | ||
#else | ||
// Always return a value. | ||
return buf[pos]; | ||
#endif |
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.
Same question as for getIndirection()
a few lines up
@rhornung67 -- would you be ok w/ merging this PR and then renaming it/incorporating it into the work you've done in #1196? |
@BradWhitlock a while back, I was toying around with a related issue (see #1196). Would that be an easier way to fix this type of issue? |
Sure. I need to get back to that, but I keep getting distracted to other things. @BradWhitlock I'm going to tag this PR in my PR so I can remember what's what. |
@rhornung67 - Adding a full-blown axom::numeric_limits<> sounds useful to me. I wasn't really aware of that work in #1196 so I used a related axom::numerics::floating_point_limits<>::max() function that was in the code today. I changed the numeric_limits in 2 places that were marked AXOM_HOST_DEVICE. |
Minor compilation warning removal
Note - there are still many more warnings where host code is called from host-device functions. This matters for CUDA/HIP systems where functions need to be available on the GPU, though not all Axom functionality needs to probably be on the GPU. It seems like AXOM_HOST_DEVICE support is being added gradually to Axom so we have a mix of these warnings. The worst offenders are slam (various Maps) and spin.
I looked at some of the slam issues that caused the host from host_device warning to appear. Many come from constructors/destructors and other methods for maps and iterators not yet having AXOM_HOST_DEVICE. Should they even? I did not include those changes because the changes started to feel like a slippery slope.