[sycl-post-link] Generate spec constants device image property even for non-native spec constants

[AlexeySachkov]

The main purpose of this change is to emit the same device image property SYCL/specialization constants not only when native spec constants are supported, but also when they are emulated.

This is needed to support SYCL 2020 specialization constants, design document can be found in: #3331.

The task is achieved by changing the way how SpecConstantsPass works with metadata: in order to provide info which should be put into device image properties, it was attached as metadata to __spirv_SpecConstant intrinsics and they are not generated if native spec constants are not available. New approach creates a single named metadata sycl.specialization-constants which contains a list of metadatas, corresponding to each specialization constant found in the module.

Such new representation makes it easier to generate metadata and more significantly, it should improve the speed of reading them, because we don't need to look over each call to __spirv_SpecConstant intrinsic.

[vladimirlaz]

It looks like it is better to move the test to clang tests and implement some stub for SYCL RT classes to let it build.

[AlexeySachkov]

Good point, will move it there

[AlexeySachkov]

After thinking about this a bit more, I'm not sure that it will work there: clang doesn't have dependency on sycl-post-link and llvm-spirv, so it would be a significant change there, while SYCL LIT tests already depend on sycl-toolchain, which includes both tools.

Also, this seems like an integration test and I'm not sure that we have a better place for it. Note: it is not a complete E2E test, so I'm not sure that llvm-test-suite is a proper place for it either

[vmaksimo]

LGTM except small issues already mentioned above

[AlexeySachkov]

@kbobrovs, could you please take a look?

[kbobrovs]

why not #include the header?

[AlexeySachkov]

The idea is to minimize amount of includes and therefore speedup compilation.

I agree that in this particular case it probably doesn't make any difference, as the header is only used twice (by SpecConstants.cpp and sycl-post-link.cpp), but nevertheless, I just applied this approach here.

[kbobrovs]

I see. llvm::Module and llvm::PreservedAnalyses would have to also be handled in similar way, and this approach does not scale. But consider this a nit, I don't see any harm in this case.

[AlexeySachkov]

and this approach does not scale.

Yes, it indeed requires some effort

I see. llvm::Module and llvm::PreservedAnalyses would have to also be handled in similar way,

We can't do so, because they are passed by reference and therefore required to be fully defined.

Note, this is a common approach in LLVM project: llvm/llvm-project@43d6f9a, llvm/llvm-project@3caa03e

[bader]

Please, read LLVM's documentation - coding standards in particular.
https://llvm.org/docs/CodingStandards.html#minimal-list-of-includes

#include hurts compile time performance. Don’t do it unless you have to, especially in header files.

But wait! Sometimes you need to have the definition of a class to use it, or to inherit from it. In these cases go ahead and #include that header file. Be aware however that there are many cases where you don’t need to have the full definition of a class. If you are using a pointer or reference to a class, you don’t need the header file. If you are simply returning a class instance from a prototyped function or method, you don’t need it. In fact, for most cases, you simply don’t need the definition of a class. And not #includeing speeds up compilation.

[AlexeySachkov]

Please, read LLVM's documentation - coding standards in particular.
https://llvm.org/docs/CodingStandards.html#minimal-list-of-includes

Thanks! I was looking for it as well. It seems like my understanding about references is wrong, I will probably send some follow-up refactoring patches

[kbobrovs]

Right, then this still holds:

llvm::Module and llvm::PreservedAnalyses would have to also be handled in similar way

This, for example, compiles just fine, as there is no real use of the object:

class Foo;
void foo(Foo&);