XNNPACK icon indicating copy to clipboard operation
XNNPACK copied to clipboard

Add subgraph support for FP16 tensors

Open GregoryComer opened this issue 1 year ago • 6 comments

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.

GregoryComer avatar Feb 06 '24 14:02 GregoryComer

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.

google-cla[bot] avatar Feb 06 '24 14:02 google-cla[bot]

This is awesome. Thanks @GregoryComer.

digantdesai avatar Feb 06 '24 14:02 digantdesai

@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!

GregoryComer avatar Feb 07 '24 17:02 GregoryComer

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

alankelly avatar Feb 09 '24 08:02 alankelly

The bazel fix has landed so if you rebase it should work.

alankelly avatar Feb 09 '24 16:02 alankelly

@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.

GregoryComer avatar Feb 09 '24 18:02 GregoryComer

Hi, can you please rebase? There are conflicts.

alankelly avatar Feb 20 '24 08:02 alankelly

Sure, I've rebased and everything looks clean.

GregoryComer avatar Feb 20 '24 22:02 GregoryComer

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.

GregoryComer avatar Feb 20 '24 22:02 GregoryComer

Done.

GregoryComer avatar Feb 20 '24 23:02 GregoryComer

@alankelly Is there anything else blocking this PR from merging? There's no rush, just want to ensure I'm not missing anything. Thanks.

GregoryComer avatar Feb 23 '24 17:02 GregoryComer

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));

alankelly avatar Feb 23 '24 19:02 alankelly

Thanks. I'll update the PR to address.

GregoryComer avatar Feb 23 '24 19:02 GregoryComer

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.

GregoryComer avatar Feb 23 '24 21:02 GregoryComer

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.

GregoryComer avatar Feb 27 '24 08:02 GregoryComer