[SYCL][Graph] Specify API for explicit update using indices

[Bensuo]

• Adds APIs to the specification for updating graph nodes using explicit indices (from set_arg() etc.)
• Also includes functionality for updating ND-range of kernel nodes
• Note: Current design is only for kernel execution nodes

Co-authored-by: Ewan Crawford [email protected] Co-authored-by: Pablo Reble [email protected]

[Bensuo]

I think this generally looks good. I was torn as to whether it's worth having a class like dynamic_parameter at this point. I think a class like that will be useful when you add support for updating kernel arguments that are captured by a lambda, but you don't have that support yet.

A simpler solution for now would be to remove dynamic_parameter and instead add a member function on node like:

class node {
  template<typename T>
  void update_arg(int argIndex, T&& arg);

I do like the way you can use the same dynamic_parameter to update kernel arguments in several nodes. Obviously, you would lose this by removing dynamic_parameter. However, dynamic_paramter adds a lot of verbosity, and I'm not sure that outweighs this one use case.

If it were me, I think I would go with the simple solution for now and consider adding dynamic_parameter later when you have a solution for lambda arguments. However, I've also added comments below that are relevant if you decide to keep dynamic_parameter.

@gmlueck Thanks for your feedback!

The main advantage I see of having the dynamic_parameter class templated on the argument type is that it enables us to constrain and check the type when updating. If we don't we have only very limited information to check that the new value is valid for the type of the argument, most likely only the size of the argument for anything except accessors. Apart from what I see as a very niche case where maybe the user intentionally wants to swap a kernel arg of one type for one of identical size but different type, it seems a bit error prone without the type checking.

Coupled with the advantage of updating many nodes at once as you mentioned I think it's worth keeping this approach for now. I don't believe this is an unreasonable amount of verbosity compared to other comparable APIs.

[EwanC]

Taking this out of draft now that the implementation has been merged in https://github.com/intel/llvm/pull/12840

[EwanC]

This looks good! Just a couple small comments below.

@gmlueck Can I confirm you're good to merge this? The implementation of the API defined in this PR is merged now and the comment threads here are addressed, so from my perspective it's good to go.

[gmlueck]

Can I confirm you're good to merge this? The implementation of the API defined in this PR is merged now and the comment threads here are addressed, so from my perspective it's good to go.

LGMT!

[EwanC]

@intel/llvm-gatekeepers Can you merge this please, thanks