[HIP] Remove reinstation of context

[hdelan]

Same as intel/llvm#4442 but for HIP. A lot of the conversation in the linked PR isn't relevant since we now use the primary context in both HIP and CUDA.

The main reason for this change is that we don't need to reinstate the context at every ScopedContext destruction, since any operation that relies on having a native context set uses a ScopedContext, and doesn't rely on any sort of implicit context stack. Making this change allows us to potentially remove a single HIP API call from each ScopedContext use.

[hdelan]

intel/llvm CI here intel/llvm#11617

[jchlanda]

This does more than "Remove reinstation of context", the lifetime of hipCtx_t Original used to be the span of ScopedContext, whereas now it is only local to the constructor, not sure if you meant that?

[hdelan]

This does more than "Remove reinstation of context", the lifetime of hipCtx_t Original used to be the span of ScopedContext, whereas now it is only local to the constructor, not sure if you meant that?

This is fine imo. There is no operation needed in the destructor and the ScopedContext has no other member funcs so Original only living as long as it is needed is fine.

[jchlanda]

This does more than "Remove reinstation of context", the lifetime of hipCtx_t Original used to be the span of ScopedContext, whereas now it is only local to the constructor, not sure if you meant that?

This is fine imo. There is no operation needed in the destructor and the ScopedContext has no other member funcs so Original only living as long as it is needed is fine.

I see, that's fine by me. One thing that I wonder about is the whole notion of ScopedContext, while it used to make sense, with the class being effectively an RAII over hipCtx_t, now it doesn't encapsulate the use. All it really does is it assures that we're using native context. I would consider renaming it to capture the intent better.

[hdelan]

I see, that's fine by me. One thing that I wonder about is the whole notion of ScopedContext, while it used to make sense, with the class being effectively an RAII over hipCtx_t, now it doesn't encapsulate the use. All it really does is it assures that we're using native context. I would consider renaming it to capture the intent better.

I agree that the ScopedContext is no longer the RAII class it used to be. Perhaps a renaming falls outside the scope of this PR but I could potentially put up another PR renaming it to something else for both CUDA and HIP adapters.

[ldrumm]

I see, that's fine by me. One thing that I wonder about is the whole notion of ScopedContext, while it used to make sense, with the class being effectively an RAII over hipCtx_t, now it doesn't encapsulate the use. All it really does is it assures that we're using native context. I would consider renaming it to capture the intent better.

I agree that the ScopedContext is no longer the RAII class it used to be. Perhaps a renaming falls outside the scope of this PR but I could potentially put up another PR renaming it to something else for both CUDA and HIP adapters.

Yeah. I'm not married to this patch, and had kind of forgotten about it. Feel free to use it, or throw it away for something better. If you want me to rename it here, I can also do that

[hdelan]

Yeah. I'm not married to this patch, and had kind of forgotten about it. Feel free to use it, or throw it away for something better. If you want me to rename it here, I can also do that

I think it's no longer worth renaming the scopedContext to scopedDevice. I think naming changes are actually difficult to merge since naming is hard and there is often no real need to rename other than "it fits better". So I am reluctant to try and rename this again. I think the current patch's changes should be able to get through (bringing it on par with CUDA adapter's ScopedContext) without doing a bigger renaming. Also I think we are the only ones really using this class so if we understand what we are doing with it then that's sort of all that matters.

[jchlanda]

Yeah. I'm not married to this patch, and had kind of forgotten about it. Feel free to use it, or throw it away for something better. If you want me to rename it here, I can also do that

I think it's no longer worth renaming the scopedContext to scopedDevice. I think naming changes are actually difficult to merge since naming is hard and there is often no real need to rename other than "it fits better". So I am reluctant to try and rename this again. I think the current patch's changes should be able to get through (bringing it on par with CUDA adapter's ScopedContext) without doing a bigger renaming. Also I think we are the only ones really using this class so if we understand what we are doing with it then that's sort of all that matters.

If the names makes more sense in Cuda and we're sticking to it in here to make things align, than I'm find with it.

[hdelan]

@kbenzie do you think we can make this ready-to-merge?