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

Remove dep. on runtime workaround in binary.cpp #2104

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

ctodTT
Copy link
Contributor

@ctodTT ctodTT commented Feb 4, 2025

This change is made to avert an innocuous bug where ttrt can't find a symbol when it is built in debug mode.

Problem description

Sometimes the symbol _ZN2tt7runtime10workaround3Env3getEbbbbb is missing from a Debug built ttrt.

What's changed

The code that introduces a dependency on that symbol has been removed

@nsmithtt
Copy link
Contributor

nsmithtt commented Feb 4, 2025

@jnie-TT in general we want to only have flatbuffer related code linked into the binary.cpp target for TTRT which is intended only for flatbuffer reflection and can be used offline (on macOS). All that said, the workarounds struct is probably a gray area since it's not strictly a standalone thing.

We initially tried to link the workarounds lib into the binary.cpp lib via setup.py, but it didn't immediately work so we opted for this simpler solution.

Copy link
Contributor

@jnie-TT jnie-TT left a comment

Choose a reason for hiding this comment

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

Thanks for the heads up, yeah I agree binary.cpp should be isolated from runtime. Left a minor comment but otherwise looks good.

@@ -39,10 +38,6 @@ static std::string asJson(void const *fbb, uint8_t const *binarySchema,

static std::vector<uint32_t>
calculateStride(std::vector<uint32_t> const &shape) {
LOG_ASSERT(
workaround::Env::get().defaultStrideComputation,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not using this anymore, maybe we should remove the workaround entirely from workaround env, ttrt etc., and move the TODO comment here

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! let me know if I missed anything, but otherwise will probably merge once CI passes

@ctodTT ctodTT requested a review from tapspatel as a code owner February 5, 2025 17:48
@ctodTT ctodTT force-pushed the ctod/fix/remove-workaround-dep branch from cb29695 to 852e16e Compare February 5, 2025 17:56
@ctodTT ctodTT force-pushed the ctod/fix/remove-workaround-dep branch from 852e16e to aeeb0a7 Compare February 5, 2025 19:19
This change is made to avert an innocuous bug where `ttrt` can't find a
symbol when it is built in debug mode.
@ctodTT ctodTT force-pushed the ctod/fix/remove-workaround-dep branch from aeeb0a7 to 26a66d2 Compare February 6, 2025 16:43
@ctodTT ctodTT merged commit be59322 into main Feb 6, 2025
30 checks passed
@ctodTT ctodTT deleted the ctod/fix/remove-workaround-dep branch February 6, 2025 22:07
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