[SYCL] Update aspect propagation for virtual functions

[jzc]

spec: #10540

[sarnex]

not an expert on this feature but makes sense, just minor comments

[sarnex]

Any reason we are using std::vector over SmallVector or some other LLVM data structure?
Also we might want to make a type alias so the type is cleaner.

[AlexeySachkov]

It should likely be SmallVector. At least for sequential containers LLVM Programmer's Manual is clear that you should pick the first one which suites your needs and std::vector is at the very end of the list

[sarnex]

should we check the aspects for subFoo and subBar too?

[AlexeySachkov]

We could, but that part should be already covered by tests for "core" aspects propagation mechanism. Those tests are specially focused on scenarios where a kernel does not use a function directly, but still inherits its aspects, because it uses a "set" where this function belongs to.

[AlexeySachkov]

If it won't require refactoring of existing code (due to shared data structures, or functions), I would add const to F argument (and everywhere else where it is applicable, i.e. to processDeclaredVirtualFunctionSets arguments as well)

[AlexeySachkov]

Should we put those into a subfolder to make codebase cleaner (in case we add more tests and in general)?

[AlexeySachkov]

We could, but that part should be already covered by tests for "core" aspects propagation mechanism. Those tests are specially focused on scenarios where a kernel does not use a function directly, but still inherits its aspects, because it uses a "set" where this function belongs to.

[AlexeySachkov]

Design doc also states the following:

NOTE: if the aspects propagation pass is ever extended to track function
pointers, then aspects attached to virtual functions should not be attached
to kernels using this mechanism. For example, if a kernel uses a variable,
which is initialized with a function pointer to a virtual function which uses
an aspect, then such kernel should not be considered as using that aspect.
Properties-based mechanism which is described above should be used for aspects
propagation for virtual functions.

The idea was to support the case like this one:

struct set_fp64;
struct set_fp32;

struct Foo {
  virtual SYCL_EXT_ONEAPI_FUNCTION_PROPERTY(indirectly_callable_in<set_fp64>)
  void bar() {
    double d;
  }

  virtual SYCL_EXT_ONEAPI_FUNCTION_PROPERTY(indirectly_callable_in<set_fp32>)
  void baz() {
    float f;
  }
};

int main() {
  queue q;
  auto *ptr = malloc_device<Foo>(1, q);
  q.single_task<class Construct>([=]() {
    new (ptr) Foo;
  });
}

Construct kernel could be launched on a device which doesn't support fp64 and it shouldn't result in any errors, i.e. even though Construct kernel under the hood operates with a vtable which references Foo::bar, the kernel shouldn't be considered to use fp64 aspect.

That should be true already, because propagating aspects like that would require extra code, but I think that we should add a test to indicate to us that such addition should not be performed. Or, even if it is performed, then it should ignore virtual functions, because they use their own special mechanism.