Skip to content

[NFC][RA] Refactor RABasic into a Separate Header #149555

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

Merged
merged 2 commits into from
Jul 19, 2025

Conversation

kyulee-com
Copy link
Contributor

This change refactors the RABasic type by moving it from RegAllocBasic.cpp to a new header file, RegAllocBasic.h. This separation of header and implementation aligns with the structure used by other register allocators, such as RegAllocGreedy. The refactoring is intended to facilitate future use of RABasic in other contexts.

This change refactors the RABasic type by moving it from RegAllocBasic.cpp to a new header file, RegAllocBasic.h. This separation of header and implementation aligns with the structure used by other register allocators, such as RegAllocGreedy. The refactoring is intended to facilitate future use of RABasic in other contexts.
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.

In principle I'm fine with this. What's the uses you have in mind? Will it be used upstream?

Comment on lines 9 to 10
// This file declares the RABasic class, which provides a minimal
// implementation of the basic register allocator.
Copy link
Contributor

Choose a reason for hiding this comment

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

use syntax recognized by doxygen (and possibly fix the pre-existing problem in RegAllocBasic.cpp)

Suggested change
// This file declares the RABasic class, which provides a minimal
// implementation of the basic register allocator.
/// \file This file declares the RABasic class, which provides a minimal
/// implementation of the basic register allocator.

Comment on lines 14 to 15
#ifndef LLVM_CODEGEN_REGALLOCBAISC_H_
#define LLVM_CODEGEN_REGALLOCBAISC_H_
Copy link
Contributor

@MatzeB MatzeB Jul 18, 2025

Choose a reason for hiding this comment

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

Suggested change
#ifndef LLVM_CODEGEN_REGALLOCBAISC_H_
#define LLVM_CODEGEN_REGALLOCBAISC_H_
#ifndef LLVM_CODEGEN_REGALLOCBASIC_H
#define LLVM_CODEGEN_REGALLOCBASIC_H

Copy link
Contributor

Choose a reason for hiding this comment

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

most files tend to use LLVM_LIB_CODEGEN_XXXX_H it seems...

static char ID;
};
} // namespace llvm
#endif // #ifndef LLVM_CODEGEN_REGALLOCBAISC_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#endif // #ifndef LLVM_CODEGEN_REGALLOCBAISC_H_
#endif

Comment on lines 19 to 24
#include "llvm/CodeGen/LiveIntervals.h"
#include "llvm/CodeGen/LiveRangeEdit.h"
#include "llvm/CodeGen/LiveRegMatrix.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
#include "llvm/CodeGen/Spiller.h"
#include "llvm/CodeGen/VirtRegMap.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check which include files are actually needed, and which ones can be replaced with class xxx; forward declarations. LiveRegMatrix and VirtRegMap seem unused at a first glance...

@kyulee-com
Copy link
Contributor Author

In principle I'm fine with this. What's the uses you have in mind? Will it be used upstream?

@MatzeB Thanks for the review! Incorporated the feedbacks.

I’m currently working on a prototype for a wrapper RA that dispatches different RAs based on function profiles or heuristics. Depending on how complete and generally applicable it becomes, this feature could be upstreamed in the future.

@MatzeB
Copy link
Contributor

MatzeB commented Jul 18, 2025

Going to accept. LGTM

(Though admittedly I feel we should rather understand why exactly regallocbasic performs better in the "codesize after compression" metric and aim at tweaking greedy alloc instead of adding higher level complexity...)

@kyulee-com kyulee-com merged commit 52bcc7b into llvm:main Jul 19, 2025
9 checks passed
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025
This change refactors the RABasic type by moving it from
RegAllocBasic.cpp to a new header file, RegAllocBasic.h. This separation
of header and implementation aligns with the structure used by other
register allocators, such as RegAllocGreedy. The refactoring is intended
to facilitate future use of RABasic in other contexts.
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