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

[RegAllocFast] Refactor dominates algorithm for large basic block #72250

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

HaohaiWen
Copy link
Contributor

The original brute force dominates algorithm is O(n) complexity so it is
very slow for very large machine basic block which is very common with
O0. This patch added InstrPosIndexes to assign index for each
instruction and use it to determine dominance. The complexity is now
O(1).

The original brute force dominates algorithm is O(n) complexity so it is
very slow for very large machine basic block which is very common with
O0. This patch added InstrPosIndexes to assign index for each
instruction and use it to determine dominance. The complexity is now
O(1).
@HaohaiWen
Copy link
Contributor Author

Hi, is there any more comment? I'd like to land this patch.

Comment on lines +116 to +119
for (auto I = Start; I != End; ++I) {
LastIndex += Step;
Instr2PosIndex[&*I] = LastIndex;
}
Copy link
Contributor

@phoebewang phoebewang Dec 12, 2023

Choose a reason for hiding this comment

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

Do we know when new MI being inserted? I think we can simply assgin it a value if we do, e.g.,

MachineInstr *New = BuildMI(MBB, II, ...
uint64_t NewPos = Instr2PosIndex[&*II] - 1;
if (LLVM_UNLIKELY(NewPos & (InstrDist - 1) == 0)) ...
Instr2PosIndex[New] = NewPos;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This requires to carefully maintain this PosIndex for each insertion point. I think it's unfriendly for other users.
If we insert two instructions before same place, we need to be careful about the step to avoid duplication.

Copy link
Contributor

@yubingex007-a11y yubingex007-a11y left a comment

Choose a reason for hiding this comment

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

LGTM

@HaohaiWen HaohaiWen merged commit 40ec791 into llvm:main Dec 22, 2023
4 checks passed
@HaohaiWen HaohaiWen deleted the fastra branch December 22, 2023 15:06
for (const MachineInstr &MI : MBB) {
LastIndex += InstrDist;
Instr2PosIndex[&MI] = LastIndex;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this lazily, on first query?

It looks like dominates() is actually only used in very rare situations (where a block branches back to itself), so populating this map is unnecessary for most blocks.

Copy link
Contributor Author

@HaohaiWen HaohaiWen Dec 23, 2023

Choose a reason for hiding this comment

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

Thanks for suggestion. See #76275

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.

6 participants