Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup ckernel SFPU includes and remove using namespace #31

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JushBJJ
Copy link

@JushBJJ JushBJJ commented Mar 1, 2025

Ticket
tenstorrent/tt-metal#7736 & #20

What this PR is about
This an initial PR to check whether what I'm doing aligns with what you guys also prefer before i start continue with cleaning up for Wormhole and Blackhole. It removes includes that don't need to be in ckernel_sfpu_*.h headers in both:

  • tt_metal/hw/ckernels/grayskul/metal/llk_api/llk_sfpu/ for the tt-metal repo, no PR for this yet until this one is pushed.
  • tt-llk/tt_llk_grayskull/common/inc/sfpu/ in this repo

I also voluntarily removed using namespace sfpi; to help a bit with #20, only keeping when its really needed.

Testing
So far with the tests I've done have shown no errors with the changes made, but would like some cross-verification.

The two commands i used for testing is just this:
./build/test/tt_metal/unit_tests_llk
and
./build/test/tt_eager/ops/test_sfpu

and some internal testing with compute kernels like including the file and testing whether their functions at least execute or not without any errors.

SFPU headers modified

Operation Grayskull Wormhole Blackhole
abs
gelu
exp
identity
log
recip
relu
sigmoid
sign
sqrt
square
tanh
clamp
comp
dropout*
hardtanh
is_fp16_zero
max
power
tanh_derivative
topk
trigonometry
reverseops
add1
binop_with_unary
cdf
converter
elu
erf_erfc
erfinv
exp2
expm1
fill
heaviside
i0
i1
isinf_isnan
logical_not_noti
mask
min
negative
power_iterative
rsqrt
sigmoid_appx
signbit
silu
tiled_prod
unary_comp

#include "ckernel.h"
#include "noc_nonblocking_api.h"

#include "sfpi.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we do need sfpi.h, since dst_reg is actually sfpi::dst_reg, defined in the header in question.

#include "ckernel.h"
#include "noc_nonblocking_api.h"

#include "sfpi.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to include this header for the vFloat definition (as well as dst_reg).
Additionally, we're missing #include "sfpi_fp16.h", which provides the s2vFloat16 definition.

The goal is to explicitly include the necessary headers to prevent reliance on transitive dependencies, ensuring the code remains robust and doesn't break by chance.

@JushBJJ
Copy link
Author

JushBJJ commented Mar 4, 2025

What do you think now? Should something like

#include "grayskull/sfpi_lib.h"

stay or we remove it and depend on sfpi.h to automatically do it for us because it has this:

#if defined(ARCH_GRAYSKULL)
#include <grayskull/sfpi_imp.h>
#include <grayskull/sfpi_lib.h>
#elif defined(ARCH_WORMHOLE)
#include <wormhole/sfpi_imp.h>
#include <wormhole/sfpi_lib.h>
#elif defined(ARCH_BLACKHOLE)
#include <blackhole/sfpi_imp.h>
#include <blackhole/sfpi_lib.h>
#endif

@fvranicTT
Copy link
Contributor

What do you think now? Should something like

#include "grayskull/sfpi_lib.h"

stay or we remove it and depend on sfpi.h to automatically do it for us because it has this:

#if defined(ARCH_GRAYSKULL)
#include <grayskull/sfpi_imp.h>
#include <grayskull/sfpi_lib.h>
#elif defined(ARCH_WORMHOLE)
#include <wormhole/sfpi_imp.h>
#include <wormhole/sfpi_lib.h>
#elif defined(ARCH_BLACKHOLE)
#include <blackhole/sfpi_imp.h>
#include <blackhole/sfpi_lib.h>
#endif

I'd say let's depend on sfpi header.

I've looked into namespace cleanup. It seems to be more involved than we initially thought, as it requires actual hardware. LLK compilation is, in fact, a "just-in-time" operation, and you can't test the changes without a device, since LLK code compiles before running on the hardware.

You can either check these changes by running them on the device locally or by running a GitHub Action that essentially does the same thing. However, GitHub Actions can only be started by TT internal folks.

So, for now, let's try to clean up superfluous header files. That will also require assistance from the internal team, and we can help with running checks in tt-metal repo.

@JushBJJ
Copy link
Author

JushBJJ commented Mar 4, 2025

pretty sure this is all good, if everything's okay with you now i can just do wormhole and blackhole as well in this PR before pushing. Then i can move on to the ckernels over at the tt-metal repo

So, for now, let's try to clean up superfluous header files. That will also require assistance from the internal team, and we can help with running checks in tt-metal repo.

yeah, definitely. Tests on wormhole are kinda broken at the moment (tenstorrent/tt-metal#18542) and blackhole is going to be pure guess work for me.

@JushBJJ JushBJJ changed the title Cleanup ckernel SFPU includes and remove using namespace (Grayskull) Cleanup ckernel SFPU includes and remove using namespace Mar 6, 2025
@nvelickovicTT
Copy link
Contributor

@JushBJJ go ahead and do it for WH and BH as well. It should actually make it easier to test as these architectures are better supported than GS.

@fvranicTT
Copy link
Contributor

@JushBJJ please ping me once you think you're done. I will start a build for you in our build pipeline. In fact I just did, and then I saw your revert, so I stopped it :)

@JushBJJ
Copy link
Author

JushBJJ commented Mar 7, 2025

Thanks, I reverted it because there was a lot of stuff that changed how the LLK code worked that went over my radar (even though the tests passed on my n300 instance).

Could you run for BH though for #43? if it doesnt pass dropout lmk

@JushBJJ
Copy link
Author

JushBJJ commented Mar 7, 2025

@fvranicTT You can run this through the Wormhole CI now. i'll do blackhole tomorrow

btw do you know how to fix these merge conflicts, tried rebasing but didnt work maybe i didnt do it properly?

Most of the conflicts are likely from the linting you did that I haven't applied yet in this branch

@JushBJJ
Copy link
Author

JushBJJ commented Mar 7, 2025

Blackhole also done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants