Add subgraph support for FP16 tensors
As per previous design discussions, update subgraph node definition methods to allow for FP16 inputs. Add test coverage for FP16 support using subgraph API.
Tested building with cmake and running tests via cmake:
cmake --build . -j32 && ctest -j32 --output-on-failure
...
100% tests passed, 0 tests failed out of 382
This is a fairly large PR, so feel free to suggest that I break it up, if preferred for easier review. As an additional note, I did notice that there is a significant amount of boilerplate code in many of the tests, I've followed the example of existing FP32/Q8 tests, but would be happy to refactor to reduce code duplication, if desired.
Also, bazel build appears to be broken (local errors match error in CI bazel build), so I have not tested via bazel. If that issue is resolved, I can go ahead and verify that it builds and runs via bazel.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
View this failed invocation of the CLA check for more information.
For the most up to date status, view the checks section at the bottom of the pull request.
This is awesome. Thanks @GregoryComer.
@alankelly
I've rebased on top of the latest master commit and updated usages. I also added test coverage for fp16 convert, which I previously missed. I also addressed the issue with fp16.h import in concatenate5.cc. Tests and build are all passing locally (via cmake), so I believe everything has been addressed and is ready for review. Thanks!
Sorry for the broken bazel build, someone is working on it internally.
Can you please add "@FP16" to all the dependencies of all the tests you touched in test/BUILD.bazel
The bazel fix has landed so if you rebase it should work.
@alankelly
I've rebased and updated the bazel test targets. I can build and run op tests locally with bazel, so everything looks good on my end.
Hi, can you please rebase? There are conflicts.
Sure, I've rebased and everything looks clean.
Actually, looks like during rebase some extra commits ended up getting pulled in somehow. I'll clean it up and then we should be good to go from my end.
Done.
@alankelly Is there anything else blocking this PR from merging? There's no rush, just want to ensure I'm not missing anything. Thanks.
Sorry forgot about this. There are some failing tests:
Add2, divide2, maximum2, multiply2, squared_difference, subtract2 fail now, looks like the new f16 tests needs:
ASSERT_EQ(xnn_status_success, xnn_initialize(/allocator=/nullptr));
Thanks. I'll update the PR to address.
Hi @alankelly, regarding the test failures, are these internal tests? I'm not seeing any test failures running via cmake/ctest on linux/x86-64. Looking at divide2.cc, for example, I do see the xnn_initialize calls in both of the new fp16 tests. It does seem like the tests you mentioned do use the BinaryTest fixture, so maybe there's something there. Could you provide any info on the specific failures you're seeing, such as a stack trace or error message? Thanks.
I just pre-emptively rebased again. Looked like there was one conflict that needed to be addressed. Tests pass locally. I still do need a little more information on the test failures mentioned above.