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

#12323: delete ctor of AllGatherFusedOpSignaler #12324

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

marty1885
Copy link
Contributor

@marty1885 marty1885 commented Sep 6, 2024

Ticket

Link to Github Issue

Problem description

Due to the ctor of AllGatherFusedOpSignaler is essentially the default constructor. The expression std::optional<AllGatherFusedOpSignaler> all_gather_fused_op_signaler = AllGatherFusedOpSignaler(); triggers GCC14's uninitialized variable detection.

What's changed

Delete the default constructor.

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

Copy link
Member

@ayerofieiev-tt ayerofieiev-tt left a comment

Choose a reason for hiding this comment

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

Or this constructor has to be removed

    MatmulFusedOpSignaler() {}

@marty1885
Copy link
Contributor Author

Oops.. forced pushed the wrong commit and GH auto closed the PR. Fixed now.

@ayerofieiev-tt Still compiling on my machine but I think your solution is better.

@marty1885 marty1885 changed the title #12323: apply default values to MatmulFusedOpSignaler #12323: delete ctor of AllGatherFusedOpSignaler Sep 6, 2024
@marty1885
Copy link
Contributor Author

@ayerofieiev-tt This patch works well on my side. Can you help merge it into main?

@tt-rkim
Copy link
Collaborator

tt-rkim commented Sep 9, 2024

@TT-billteng @ttmchiou Looks like gcc 14 was released literally this year. We're still quite limited on runners. Should we keep it to gcc-12 that's tested on CI? Nightlies etc?

@marty1885
Copy link
Contributor Author

marty1885 commented Sep 9, 2024

@tt-rkim I understand TT's difficulties keeping up with new compilers. I also have no problem keep upstreaming these patches when breaks do happen. The last thing I want is maintaining a branch with piles of patches just so I can get tt-metal working on my machine but TT still gotta reinvent the wheel when the next version of Ubuntu released. It also complicates the process of me upstreaming bug fixes as things have to be backported instead of a clean branch.

Can TT accept these patches as-is and we keep an eye on gcc-12 compatibility?

@tt-rkim
Copy link
Collaborator

tt-rkim commented Sep 9, 2024

Just running this to be sure: https://github.com/tenstorrent/tt-metal/actions/runs/10775571014

@tt-rkim
Copy link
Collaborator

tt-rkim commented Sep 9, 2024

And we are okay with that workflow, if you are okay with it

@marty1885
Copy link
Contributor Author

marty1885 commented Sep 9, 2024

I would prefer if TT can have newer GCC in CI. But open source is where downstream developers will do anything and everything to get their code working. And expect upstream to accept the patches unless there's a good reason not to. Mutual help.

So yes, I'm happy keep upstreaming these fixes if TT doesn't have the resource.

@tt-rkim tt-rkim merged commit 8d4af62 into tenstorrent:main Sep 10, 2024
6 checks passed
@tt-rkim
Copy link
Collaborator

tt-rkim commented Sep 10, 2024

Once we get more compute resources, we should be able to support more GCC releases. I'm wondering given the stage we are currently in, if it's worth just upgrading our gcc across the board and requiring 14 or higher.

@TT-billteng
Copy link
Collaborator

which OS natively supports GCC 14 currently?

@marty1885
Copy link
Contributor Author

marty1885 commented Sep 12, 2024

Ubuntu 24.04 (package: gcc-14), Fedora 40, Arch Linux

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.

4 participants