[PATCH] D94013: [libclc] Add clspv target for libclc

[Daniel Stone via Phabricator via llvm-commits]

daniels added a subscriber: jenatali.
daniels added a comment.

> On the surface it looks like the only thing preventing you from using builtins.opt.spirv-mesa3d-.bc is the custom implementation of fma and nextafter plus using -O3 optimization.

Well, that and the fact that they emit SPIR (i.e. LLVM IR) rather than SPIR-V (to which its only relation is nomenclature). I haven't actually looked at clspv at all recently, but presumably they combine the LLVM IR (aka SPIR) from here together with the LLVM IR gained from running Clang and link them there. Mesa is much much less LLVM-centric, so we want to avoid LLVM IR where possible, hence our choice of SPIR-V output instead.

If the fma implementation still passes the full OpenCL regular-profile CTS then it might be interesting to reuse that implementation in Mesa as well, since as you say, it's going to be a great deal faster (cc @jenatali). Everything else looks reasonable to me & I think it makes sense to merge. Welcome to the layered-CL family!



================
Comment at: libclc/generic/lib/gen_convert.py:358
     print("  {DST}{N} r = convert_{DST}{N}(x);".format(DST=dst, N=size))
-    print("  {SRC}{N} y = convert_{SRC}{N}(y);".format(SRC=src, N=size))
+    print("  {SRC}{N} y = convert_{SRC}{N}(r);".format(SRC=src, N=size))
     if mode == '_rtz':
----------------
tstellar wrote:
> alan-baker wrote:
> > tstellar wrote:
> > > Why did this change?
> > I can revert this since I don't use the conversion builtins, but the code this generates is obviously wrong. y is being used in its own declaration. This leads to llvm generating undefs in the conversion functions.
> Ok, this would make sense as a separate patch.
D81999 has a fix for this already, but I wasn't able to test it, because the SPIR-V output completely bypasses the provided functions in favour of fixed opcodes/decorations/internal-calls anyway


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94013/new/

https://reviews.llvm.org/D94013