[RISCV][NewPM] Port RISCVCodeGenPrepare to the new pass manager #168381
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As suggested in the review for llvm#160536 it would be good to follow up and port the RISC-V passes to the new pass manager. This PR starts that task. It provides the bare minimum necessary to run RISCVCodeGenPrepare with opt -passes=riscv-codegenprepare. The approach used is modeled on my observations of the AMDGPU backend and the recent work to port the X86 passes. One unfortunate annoyance is that the decision to split the passes into RISCVFoo (holds logic for implementing the pass), RISCVFooLegacyPass (provides the legacy pass manager interface and calls into RISCVFoo), and RISCVFooPass (provides the new pass manager interface and calls into RISCVFoo) means we end up with a `initializeRISCVCodeGenPrepareLegacyPassPass` function. X86 has the same thing and lives with it. I suggest doing the same here. The naming scheme is logical to me, and it doesn't seem worth messing with the PassSupport.h macros. I would appreciate a close review for the overall approach, as I intend to follow up with patches in a similar style for the other RISC-V passes. i.e. if you think I've somehow picked up on less desirable naming / structuring or other features from the X86 and AMDGPU passes I've looked at, now is the best time to speak up!
asb
requested review from
arsenm,
boomanaiden154,
paperchalice,
preames and
topperc
Member
|
@llvm/pr-subscribers-backend-risc-v Author: Alex Bradbury (asb) ChangesAs suggested in the review for #160536 it would be good to follow up and port the RISC-V passes to the new pass manager. This PR starts that task. It provides the bare minimum necessary to run RISCVCodeGenPrepare with opt -passes=riscv-codegenprepare. The approach used is modeled on my observations of the AMDGPU backend and the recent work to port the X86 passes. One unfortunate annoyance is that the decision to split the passes into RISCVFoo (holds logic for implementing the pass), RISCVFooLegacyPass (provides the legacy pass manager interface and calls into RISCVFoo), and RISCVFooPass (provides the new pass manager interface and calls into RISCVFoo) means we end up with a I would appreciate a close review for the overall approach, as I intend to follow up with patches in a similar style for the other RISC-V passes. i.e. if you think I've somehow picked up on less desirable naming / structuring or other features from the X86 and AMDGPU passes I've looked at, now is the best time to speak up! The testing approach is to add a I have attempted to minimise cleanups/changes unrelated to moving to the new pass manager (e.g. Full diff: https://github.com/llvm/llvm-project/pull/168381.diff 5 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCV.h b/llvm/lib/Target/RISCV/RISCV.h
index 51e8e8574ed15..ec16806912c8f 100644
--- a/llvm/lib/Target/RISCV/RISCV.h
+++ b/llvm/lib/Target/RISCV/RISCV.h
@@ -26,8 +26,16 @@ class RISCVRegisterBankInfo;
class RISCVSubtarget;
class RISCVTargetMachine;
-FunctionPass *createRISCVCodeGenPreparePass();
-void initializeRISCVCodeGenPreparePass(PassRegistry &);
+class RISCVCodeGenPreparePass : public PassInfoMixin<RISCVCodeGenPreparePass> {
+private:
+ const RISCVTargetMachine &TM;
+
+public:
+ RISCVCodeGenPreparePass(const RISCVTargetMachine &TM) : TM(TM) {}
+ PreservedAnalyses run(Function &F, FunctionAnalysisManager &FAM);
+};
+FunctionPass *createRISCVCodeGenPrepareLegacyPass();
+void initializeRISCVCodeGenPrepareLegacyPassPass(PassRegistry &);
FunctionPass *createRISCVDeadRegisterDefinitionsPass();
void initializeRISCVDeadRegisterDefinitionsPass(PassRegistry &);
diff --git a/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp b/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp
index ce349598bd9b1..e983291767b8f 100644
--- a/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp
+++ b/llvm/lib/Target/RISCV/RISCVCodeGenPrepare.cpp
@@ -33,20 +33,33 @@ using namespace llvm;
#define PASS_NAME "RISC-V CodeGenPrepare"
namespace {
-
-class RISCVCodeGenPrepare : public FunctionPass,
- public InstVisitor<RISCVCodeGenPrepare, bool> {
+class RISCVCodeGenPrepare : public InstVisitor<RISCVCodeGenPrepare, bool> {
+ Function &F;
const DataLayout *DL;
const DominatorTree *DT;
const RISCVSubtarget *ST;
+public:
+ RISCVCodeGenPrepare(Function &F, const DominatorTree *DT,
+ const RISCVSubtarget *ST)
+ : F(F), DL(&F.getDataLayout()), DT(DT), ST(ST) {}
+ bool run();
+ bool visitInstruction(Instruction &I) { return false; }
+ bool visitAnd(BinaryOperator &BO);
+ bool visitIntrinsicInst(IntrinsicInst &I);
+ bool expandVPStrideLoad(IntrinsicInst &I);
+ bool widenVPMerge(IntrinsicInst &I);
+};
+} // namespace
+
+namespace {
+class RISCVCodeGenPrepareLegacyPass : public FunctionPass {
public:
static char ID;
- RISCVCodeGenPrepare() : FunctionPass(ID) {}
+ RISCVCodeGenPrepareLegacyPass() : FunctionPass(ID) {}
bool runOnFunction(Function &F) override;
-
StringRef getPassName() const override { return PASS_NAME; }
void getAnalysisUsage(AnalysisUsage &AU) const override {
@@ -54,15 +67,8 @@ class RISCVCodeGenPrepare : public FunctionPass,
AU.addRequired<DominatorTreeWrapperPass>();
AU.addRequired<TargetPassConfig>();
}
-
- bool visitInstruction(Instruction &I) { return false; }
- bool visitAnd(BinaryOperator &BO);
- bool visitIntrinsicInst(IntrinsicInst &I);
- bool expandVPStrideLoad(IntrinsicInst &I);
- bool widenVPMerge(IntrinsicInst &I);
};
-
-} // end anonymous namespace
+} // namespace
// Try to optimize (i64 (and (zext/sext (i32 X), C1))) if C1 has bit 31 set,
// but bits 63:32 are zero. If we know that bit 31 of X is 0, we can fill
@@ -273,17 +279,7 @@ bool RISCVCodeGenPrepare::expandVPStrideLoad(IntrinsicInst &II) {
return true;
}
-bool RISCVCodeGenPrepare::runOnFunction(Function &F) {
- if (skipFunction(F))
- return false;
-
- auto &TPC = getAnalysis<TargetPassConfig>();
- auto &TM = TPC.getTM<RISCVTargetMachine>();
- ST = &TM.getSubtarget<RISCVSubtarget>(F);
-
- DL = &F.getDataLayout();
- DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-
+bool RISCVCodeGenPrepare::run() {
bool MadeChange = false;
for (auto &BB : F)
for (Instruction &I : llvm::make_early_inc_range(BB))
@@ -292,12 +288,40 @@ bool RISCVCodeGenPrepare::runOnFunction(Function &F) {
return MadeChange;
}
-INITIALIZE_PASS_BEGIN(RISCVCodeGenPrepare, DEBUG_TYPE, PASS_NAME, false, false)
+bool RISCVCodeGenPrepareLegacyPass::runOnFunction(Function &F) {
+ if (skipFunction(F))
+ return false;
+
+ auto &TPC = getAnalysis<TargetPassConfig>();
+ auto &TM = TPC.getTM<RISCVTargetMachine>();
+ auto ST = &TM.getSubtarget<RISCVSubtarget>(F);
+ auto DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
+
+ RISCVCodeGenPrepare RVCGP(F, DT, ST);
+ return RVCGP.run();
+}
+
+INITIALIZE_PASS_BEGIN(RISCVCodeGenPrepareLegacyPass, DEBUG_TYPE, PASS_NAME,
+ false, false)
INITIALIZE_PASS_DEPENDENCY(TargetPassConfig)
-INITIALIZE_PASS_END(RISCVCodeGenPrepare, DEBUG_TYPE, PASS_NAME, false, false)
+INITIALIZE_PASS_END(RISCVCodeGenPrepareLegacyPass, DEBUG_TYPE, PASS_NAME, false,
+ false)
-char RISCVCodeGenPrepare::ID = 0;
+char RISCVCodeGenPrepareLegacyPass::ID = 0;
+
+FunctionPass *llvm::createRISCVCodeGenPrepareLegacyPass() {
+ return new RISCVCodeGenPrepareLegacyPass();
+}
-FunctionPass *llvm::createRISCVCodeGenPreparePass() {
- return new RISCVCodeGenPrepare();
+PreservedAnalyses RISCVCodeGenPreparePass::run(Function &F,
+ FunctionAnalysisManager &FAM) {
+ DominatorTree *DT = &FAM.getResult<DominatorTreeAnalysis>(F);
+ auto ST = &TM.getSubtarget<RISCVSubtarget>(F);
+ bool Changed = RISCVCodeGenPrepare(F, DT, ST).run();
+ if (!Changed)
+ return PreservedAnalyses::all();
+
+ PreservedAnalyses PA = PreservedAnalyses::none();
+ PA.preserveSet<CFGAnalyses>();
+ return PA;
}
diff --git a/llvm/lib/Target/RISCV/RISCVPassRegistry.def b/llvm/lib/Target/RISCV/RISCVPassRegistry.def
new file mode 100644
index 0000000000000..32c66ffdf3cc2
--- /dev/null
+++ b/llvm/lib/Target/RISCV/RISCVPassRegistry.def
@@ -0,0 +1,22 @@
+//===- RISCVPassRegistry.def - Registry of RISC-V passes --------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is used as the registry of passes that are part of the RISC-V
+// backend.
+//
+//===----------------------------------------------------------------------===//
+
+// NOTE: NO INCLUDE GUARD DESIRED!
+
+#ifndef FUNCTION_PASS
+#define FUNCTION_PASS(NAME, CREATE_PASS)
+#endif
+FUNCTION_PASS(
+ "riscv-codegenprepare",
+ RISCVCodeGenPreparePass(*static_cast<const RISCVTargetMachine *>(this)))
+#undef FUNCTION_PASS
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index 16ef67da83128..96de5c876caaf 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -118,7 +118,7 @@ extern "C" LLVM_ABI LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
initializeRISCVLateBranchOptPass(*PR);
initializeRISCVMakeCompressibleOptPass(*PR);
initializeRISCVGatherScatterLoweringPass(*PR);
- initializeRISCVCodeGenPreparePass(*PR);
+ initializeRISCVCodeGenPrepareLegacyPassPass(*PR);
initializeRISCVPostRAExpandPseudoPass(*PR);
initializeRISCVMergeBaseOffsetOptPass(*PR);
initializeRISCVOptWInstrsPass(*PR);
@@ -456,7 +456,7 @@ void RISCVPassConfig::addIRPasses() {
addPass(createRISCVGatherScatterLoweringPass());
addPass(createInterleavedAccessPass());
- addPass(createRISCVCodeGenPreparePass());
+ addPass(createRISCVCodeGenPrepareLegacyPass());
}
TargetPassConfig::addIRPasses();
@@ -628,6 +628,9 @@ bool RISCVPassConfig::addILPOpts() {
}
void RISCVTargetMachine::registerPassBuilderCallbacks(PassBuilder &PB) {
+#define GET_PASS_REGISTRY "RISCVPassRegistry.def"
+#include "llvm/Passes/TargetPassRegistry.inc"
+
PB.registerLateLoopOptimizationsEPCallback([=](LoopPassManager &LPM,
OptimizationLevel Level) {
if (Level != OptimizationLevel::O0)
diff --git a/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll b/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll
index cf5d0f107359a..c25e337777631 100644
--- a/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll
+++ b/llvm/test/CodeGen/RISCV/riscv-codegenprepare.ll
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt %s -S -riscv-codegenprepare -mtriple=riscv64 | FileCheck %s
+; RUN: opt %s -S -passes=riscv-codegenprepare -mtriple=riscv64 | FileCheck %s
; Make sure we convert the 4294967294 in for.body.preheader.new to -2 based on
; the upper 33 bits being zero by the dominating condition %cmp3.
|
boomanaiden154
approved these changes
Nov 17, 2025
topperc
reviewed
Nov 17, 2025
| } // namespace | ||
|
|
||
| namespace { | ||
| class RISCVCodeGenPrepareLegacyPass : public FunctionPass { |
Collaborator
There was a problem hiding this comment.
Can we just drop Pass from the end of the name to fix the "PassPass" issue?
There are other examples like
SelectionDAGISelLegacy, LiveDebugVariablesWrapperLegacy, LiveStacksWrapperLegacy, DXILDataScalarizationLegacy, DXILFinalizeLinkageLegacy, DXILLegalizeLegacy
Contributor
Author
There was a problem hiding this comment.
Yes that's another option that's been used in-tree. To me it makes sense that the shared class containing the logic is just "RISCVCodeGenPrepare" and we have separate RISCVCodeGenPreparePass (newpm) and RISCVCodeGenPrepareLegacyPass (oldpm) classes providing the pass manager interfaces. But I don't feel overly strongly. And hopefully the *Legacy* classes can be removed one day too.
(Sidenote: I think the "new" pass manager will be a teenager next year!).
Collaborator
There was a problem hiding this comment.
48 of the classes in lib/CodeGen use Legacy and 11 use LegacyPass. I'm not sure if that suggests an overall project preference or not.
Contributor
Author
There was a problem hiding this comment.
Or to put it another way - I agree that's a reasonable alternative. My feeling is that having the ugly auto-generated initializeFooPassPass is worth it for a more intuitive (IMHO) naming of the pass adapters. But If you feel otherwise, I'm happy to change.
Collaborator
There was a problem hiding this comment.
I don't feel strongly. I knew that this is not a new issue and was trying to figure out what had been done before.
llvm/lib/Target/RISCV/RISCV.h
Outdated
| void initializeRISCVCodeGenPreparePass(PassRegistry &); | ||
| class RISCVCodeGenPreparePass : public PassInfoMixin<RISCVCodeGenPreparePass> { | ||
| private: | ||
| const RISCVTargetMachine &TM; |
Contributor
There was a problem hiding this comment.
Use pointer to preserve value semantic.
Contributor
Author
There was a problem hiding this comment.
I've made that change. FWIW this is where I've followed in the footsteps of the AMDGPU passes.
CC @arsenm
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/20318 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As suggested in the review for #160536 it would be good to follow up and port the RISC-V passes to the new pass manager. This PR starts that task. It provides the bare minimum necessary to run RISCVCodeGenPrepare with opt -passes=riscv-codegenprepare. The approach used is modeled on my observations of the AMDGPU backend and the recent work to port the X86 passes.
One unfortunate annoyance is that the decision to split the passes into RISCVFoo (holds logic for implementing the pass), RISCVFooLegacyPass (provides the legacy pass manager interface and calls into RISCVFoo), and RISCVFooPass (provides the new pass manager interface and calls into RISCVFoo) means we end up with a
initializeRISCVCodeGenPrepareLegacyPassPassfunction. X86 has the same thing and lives with it. I suggest doing the same here. Other than this wart, the naming scheme seems intuitive to me, and it doesn't seem worth messing with the PassSupport.h macros.I would appreciate a close review for the overall approach, as I intend to follow up with patches in a similar style for the other RISC-V passes. i.e. if you think I've somehow picked up on less desirable naming / structuring or other features from the X86 and AMDGPU passes I've looked at, now is the best time to speak up!
The testing approach is to add a
-passes=riscv-fooRUN line to at least one test, if an appropriate test exists.I have attempted to minimise cleanups/changes unrelated to moving to the new pass manager (e.g.
DataLayoutwould arguably be better as a reference in the original code).