Skip to content

[IFC] Remove noinline attributes and default to indirect function calls #2049

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[IFC] Remove noinline attributes and default to indirect function calls #2049

wants to merge 1 commit into from
+4 −44

Conversation

@doru1004
Copy link
Contributor

@doru1004 doru1004 commented Nov 11, 2025 •
edited
Loading

Remove noinline attributes and default to indirect function calls.

@alex-breslow-amd
Copy link
Contributor

alex-breslow-amd commented Nov 13, 2025

@doru1004, is this ready for review or WIP?

@doru1004
Copy link
Contributor Author

doru1004 commented Nov 13, 2025

@doru1004, is this ready for review or WIP?

Yes it is now.

@doru1004 doru1004 changed the title [IFC] Remove noinline attributes [IFC] Remove noinline attributes and default to indirect function calls Nov 13, 2025
alex-breslow-amd
alex-breslow-amd reviewed Nov 13, 2025
View reviewed changes
CMakeLists.txt
option(ENABLE_MSCCLPP_FORMAT_CHECKS "Enable formatting checks in MSCCL++" OFF)
option(ENABLE_NPKIT "Enable NPKit" OFF)
option(ENABLE_IFC "Enable indirect function call" OFF)
option(ENABLE_IFC "Enable indirect function call" ON)
Copy link
Contributor

@alex-breslow-amd alex-breslow-amd Nov 13, 2025

Choose a reason for hiding this comment

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

This is just a no-op now, right? Maybe we can delete it.

Copy link
Contributor Author

@doru1004 doru1004 Nov 13, 2025

Choose a reason for hiding this comment

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

Well not quite, the code to perform direct function calls is still there I haven't removed that yet. Once we clean up any trace of direct calls we can remove this flag completely.

@nileshnegi
Copy link
Contributor

nileshnegi commented Nov 17, 2025

Should this guard be modified?
https://github.com/ROCm/rccl/blob/develop/src/init.cc#L2018

@alex-breslow-amd
Copy link
Contributor

alex-breslow-amd commented Nov 17, 2025

Should this guard be modified? https://github.com/ROCm/rccl/blob/develop/src/init.cc#L2018
Nilesh, that's a very good point. This is indeed something we should look at. Early results suggest that the current optimizations may be a regression for AllToAll on gfx950, so maybe this helps.

@nileshnegi nileshnegi added the ci:regression-detection Run through all collectives and data types to identify any performance issues label Nov 17, 2025
@ROCmMathLibrariesBot
Copy link

ROCmMathLibrariesBot commented Nov 18, 2025

regression-detection run on commit 16e4b3b

Artifacts - Results

@ROCmMathLibrariesBot
Copy link

ROCmMathLibrariesBot commented Nov 20, 2025

regression-detection run on commit ceed330

Artifacts - Results

@ROCmMathLibrariesBot
Copy link

ROCmMathLibrariesBot commented Nov 20, 2025

regression-detection run on commit c529e3d

Artifacts - Results

@doru1004 doru1004 force-pushed the use-indirect-fc branch 2 times, most recently from cc5a66e to cffbbd0 Compare November 20, 2025 22:31
@ROCmMathLibrariesBot
Copy link

ROCmMathLibrariesBot commented Nov 20, 2025

regression-detection run on commit cffbbd0

Artifacts - Results

@ROCmMathLibrariesBot
Copy link

ROCmMathLibrariesBot commented Nov 21, 2025

regression-detection run on commit 919883c

Artifacts - Results

@doru1004
Copy link
Contributor Author

doru1004 commented Nov 21, 2025

Should this guard be modified? https://github.com/ROCm/rccl/blob/develop/src/init.cc#L2018

Since it doesn't seem to have any effect on gfx950 ( I tried it ) I would say that any change we need to do to that code it can be independent of the changes in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alex-breslow-amd alex-breslow-amd alex-breslow-amd left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

ci:regression-detection Run through all collectives and data types to identify any performance issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@doru1004 @alex-breslow-amd @nileshnegi @ROCmMathLibrariesBot