[SYCL] Invalidate handler impl after finalize

[steffenlarsen]

This commit changes the handler::finalize function to invalidate the handle_impl object. This is to help ensure that further accesses to the impl is invalid. This helps ensure that picking apart the handler and stealing its members is internalized to finalize, allowing further moves without concern that future code might use the members after move, making the ownership clearer.

[steffenlarsen]

Following these changes, any access to the impl after invalidation should trigger a segfault. This should only ever be possible through SYCL developer mistakes, as copy and move of handler shouldn't be possible in user-code and finalize should happen after user-code concludes.

If we want, we could add a getImpl() function that has an assert that impl != nullptr, but it would require replacing all uses of impl with it. If we prefer that solution I suggest a follow-up making the replacements.

[slawekptak]

Considering, that this is only to address SYCL developer mistakes, I would leave it as impl. Changing this, would affect many places in the code (over 300 in handler.cpp?).

[steffenlarsen]

Now that KData is &&, I suspect we don't need the std::move here. That said, I think it is a little awkward for this to implicitly steal the args, so I would rather go remove the getArgs overload and do an explicit std::move(KData.getArgs()) here.

[slawekptak]

I agree, the explicit move seems better. The implicit one might not be obvious until someone checks the actual class implementation.

[aelovikov-intel]

Honestly, I think a much better way is for @slawekptak to actually finish his no-handler activities that must include refactoring handler to be simply storage + delegation to new no-handler APIs. I'd expect this won't be necessary after that.

[steffenlarsen]

Honestly, I think a much better way is for @slawekptak to actually finish his no-handler activities that must include refactoring handler to be simply storage + delegation to new no-handler APIs. I'd expect this won't be necessary after that.

Though I agree, will this happen before the ABI break window closes and/or will it be possible without an ABI break?

[vinser52]

So with ScoppedInvalidator, the changes are localized in handler::finalize() method. So I think we can merge them now and if needed, @slawekptak will refactor it later.

[steffenlarsen]

@aelovikov-intel pointed out that it might not work as expected until we promote the preview changes, which should happen shortly as the ABI break window is open. I will have a look and confirm.

[steffenlarsen]

In the meantime, I have to opens regarding this patch at the beginning. @aelovikov-intel & @vinser52 & @slawekptak - Feel free to give your thoughts on it.