Skip to content

Conversation

@grigorypas
Copy link
Member

MachineFunctionSplitter was missing a skipFunction() check, causing it to incorrectly split functions that should be skipped (e.g., functions with optnone attribute).

This patch adds an early skipFunction() check in runOnMachineFunction() to ensure these functions are never split, regardless of profile data availability or other splitting conditions.

// mfs-split-ehcode flag.
bool UseProfileData = MF.getFunction().hasProfileData();
if (!UseProfileData && !SplitAllEHCode)
if (!skipFunction(MF.getFunction()) && !UseProfileData && !SplitAllEHCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be:

Suggested change
if (!skipFunction(MF.getFunction()) && !UseProfileData && !SplitAllEHCode)
if (skipFunction(MF.getFunction()) || (!UseProfileData && !SplitAllEHCode))

or am I misreading the logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also vote to just put it into a separate if so the pattern looks the same for all the passes...

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. That was a bug.

@grigorypas grigorypas requested a review from MatzeB November 3, 2025 23:52
Copy link
Contributor

@MatzeB MatzeB left a comment

Choose a reason for hiding this comment

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

Change LGTM. Please keep an eye on the precommit checks (is one failing right now? for unrelated reasons?)

@grigorypas grigorypas merged commit 7398591 into llvm:main Nov 4, 2025
10 checks passed
@grigorypas grigorypas deleted the fix_function_splitter_optnone branch November 4, 2025 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants