llvm
llvm copied to clipboard
intel
[SYCL] Move bfloat support from experimental to supported.
Looks good, do we want to also move these bfloat16 math functions out of experimental also: https://github.com/intel/llvm/blob/bdd88e50f0165e339f85c9b134d49972de079370/sycl/include/sycl/ext/oneapi/experimental/builtins.hpp#L160 since they are defined in the same extension document as the main bfloat16 class?
btw there should be an accompanying PR to intel/llvm-test-suite updating corresponding tests, otherwise there will be lots of failures: For example in this test: https://github.com/intel/llvm-test-suite/blob/intel/SYCL/BFloat16/bfloat16_type.hpp.
Also FYI there is another open PR updating the bfloat16 class that it might be good to consider merging first https://github.com/intel/llvm/pull/6492.
I also have two global comments:
-
Please update the specification to use the template in (https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/template.asciidoc) and follow the instructions in (https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/README-process.md). I would like all new "supported" extensions to use the new template format.
-
There was a request from the ML team that the
bfloat16type should not be an "optional feature". Instead, they proposed that it should be allowed for any device. Devices without native support should use a fall-back routine for conversion. Couldn't we do this by following the pattern of the other math operations we have in "llvm/libdevice"? If we do this, there is no need for the aspect, because application code wouldn't need to check if a device supportsbloat16before using it.
Recently added sycl/test/matrix/matrix-bfloat16-test-use.cpp needs to be updated to remove 'experimental'.
some of the tests use from_bits though it has been made private. Also, sycl/test/extensions/bfloat16_host.cpp header file location for bfloat16.h needs to be corrected.
Example draft patch that incorporates new non-native software impls that pass tests for CUDA backend is here: https://github.com/rdeodhar/llvm/pull/1.
By the way, just in case you didn't notice, the bfloat16_type.cpp, is currently still marked xRUN on the CI for Intel (and also in my updated patch where I added CUDA support: bfloat16_type.cpp). I imagine that CI will be able to test this now for any Intel arch now that there are software implementations so it should be marked RUN instead of xRUN at the appropriate point? Also should there also be an additional compilation instruction for Intel Arch's that support native bfloat16 instructions, like there is for CUDA for sm_80 (in my updated patch)?