AMDGPU/NewPM Port SILoadStoreOptimizer to NPM #106362
Merged
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
optimisan
force-pushed
the
port-si-load-store-opt-pass
branch
from
3d8f8c6 to
dfc06f0
Compare
|
@llvm/pr-subscribers-backend-amdgpu Author: Akshat Oke (Akshat-Oke) ChangesFull diff: https://github.com/llvm/llvm-project/pull/106362.diff 7 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPU.h b/llvm/lib/Target/AMDGPU/AMDGPU.h
index c50474893eb7d5..3ed0a5eb98c408 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPU.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPU.h
@@ -40,7 +40,7 @@ FunctionPass *createSIPeepholeSDWAPass();
FunctionPass *createSILowerI1CopiesLegacyPass();
FunctionPass *createAMDGPUGlobalISelDivergenceLoweringPass();
FunctionPass *createSIShrinkInstructionsPass();
-FunctionPass *createSILoadStoreOptimizerPass();
+FunctionPass *createSILoadStoreOptimizerLegacyPass();
FunctionPass *createSIWholeQuadModePass();
FunctionPass *createSIFixControlFlowLiveIntervalsPass();
FunctionPass *createSIOptimizeExecMaskingPreRAPass();
@@ -190,8 +190,8 @@ extern char &AMDGPUMarkLastScratchLoadID;
void initializeSILowerSGPRSpillsPass(PassRegistry &);
extern char &SILowerSGPRSpillsID;
-void initializeSILoadStoreOptimizerPass(PassRegistry &);
-extern char &SILoadStoreOptimizerID;
+void initializeSILoadStoreOptimizerLegacyPass(PassRegistry &);
+extern char &SILoadStoreOptimizerLegacyID;
void initializeSIWholeQuadModePass(PassRegistry &);
extern char &SIWholeQuadModeID;
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPassRegistry.def b/llvm/lib/Target/AMDGPU/AMDGPUPassRegistry.def
index d8741b4b06a984..576668e29a6f91 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUPassRegistry.def
+++ b/llvm/lib/Target/AMDGPU/AMDGPUPassRegistry.def
@@ -97,4 +97,5 @@ FUNCTION_PASS_WITH_PARAMS(
MACHINE_FUNCTION_PASS("amdgpu-isel", AMDGPUISelDAGToDAGPass(*this))
MACHINE_FUNCTION_PASS("si-fix-sgpr-copies", SIFixSGPRCopiesPass())
MACHINE_FUNCTION_PASS("si-i1-copies", SILowerI1CopiesPass())
+MACHINE_FUNCTION_PASS("si-load-store-opt", SILoadStoreOptimizerPass())
#undef MACHINE_FUNCTION_PASS
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
index 37d6084ca1d25d..91183f2116bff1 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -34,6 +34,7 @@
#include "R600.h"
#include "R600TargetMachine.h"
#include "SIFixSGPRCopies.h"
+#include "SILoadStoreOptimizer.h"
#include "SIMachineFunctionInfo.h"
#include "SIMachineScheduler.h"
#include "TargetInfo/AMDGPUTargetInfo.h"
@@ -415,7 +416,7 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAMDGPUTarget() {
initializeSIShrinkInstructionsPass(*PR);
initializeSIOptimizeExecMaskingPreRAPass(*PR);
initializeSIOptimizeVGPRLiveRangePass(*PR);
- initializeSILoadStoreOptimizerPass(*PR);
+ initializeSILoadStoreOptimizerLegacyPass(*PR);
initializeAMDGPUCtorDtorLoweringLegacyPass(*PR);
initializeAMDGPUAlwaysInlinePass(*PR);
initializeAMDGPUSwLowerLDSLegacyPass(*PR);
@@ -1273,7 +1274,7 @@ void GCNPassConfig::addMachineSSAOptimization() {
addPass(&SIFoldOperandsID);
if (EnableDPPCombine)
addPass(&GCNDPPCombineID);
- addPass(&SILoadStoreOptimizerID);
+ addPass(&SILoadStoreOptimizerLegacyID);
if (isPassEnabled(EnableSDWAPeephole)) {
addPass(&SIPeepholeSDWAID);
addPass(&EarlyMachineLICMID);
diff --git a/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp b/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
index ddce80b2ae129e..06f3011fb3fd80 100644
--- a/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
@@ -57,6 +57,7 @@
//
//===----------------------------------------------------------------------===//
+#include "SILoadStoreOptimizer.h"
#include "AMDGPU.h"
#include "GCNSubtarget.h"
#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
@@ -104,7 +105,7 @@ struct AddressRegs {
// GFX10 image_sample instructions can have 12 vaddrs + srsrc + ssamp.
const unsigned MaxAddressRegs = 12 + 1 + 1;
-class SILoadStoreOptimizer : public MachineFunctionPass {
+class SILoadStoreOptimizer {
struct CombineInfo {
MachineBasicBlock::iterator I;
unsigned EltSize;
@@ -295,17 +296,21 @@ class SILoadStoreOptimizer : public MachineFunctionPass {
static InstClassEnum getCommonInstClass(const CombineInfo &CI,
const CombineInfo &Paired);
-public:
- static char ID;
-
- SILoadStoreOptimizer() : MachineFunctionPass(ID) {
- initializeSILoadStoreOptimizerPass(*PassRegistry::getPassRegistry());
- }
-
bool optimizeInstsWithSameBaseAddr(std::list<CombineInfo> &MergeList,
bool &OptimizeListAgain);
bool optimizeBlock(std::list<std::list<CombineInfo> > &MergeableInsts);
+public:
+ SILoadStoreOptimizer(AliasAnalysis *AA) : AA(AA) {}
+ bool run(MachineFunction &MF);
+};
+
+class SILoadStoreOptimizerLegacy : public MachineFunctionPass {
+public:
+ static char ID;
+
+ SILoadStoreOptimizerLegacy() : MachineFunctionPass(ID) {}
+
bool runOnMachineFunction(MachineFunction &MF) override;
StringRef getPassName() const override { return "SI Load Store Optimizer"; }
@@ -882,18 +887,18 @@ void SILoadStoreOptimizer::CombineInfo::setMI(MachineBasicBlock::iterator MI,
} // end anonymous namespace.
-INITIALIZE_PASS_BEGIN(SILoadStoreOptimizer, DEBUG_TYPE,
+INITIALIZE_PASS_BEGIN(SILoadStoreOptimizerLegacy, DEBUG_TYPE,
"SI Load Store Optimizer", false, false)
INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
-INITIALIZE_PASS_END(SILoadStoreOptimizer, DEBUG_TYPE, "SI Load Store Optimizer",
- false, false)
+INITIALIZE_PASS_END(SILoadStoreOptimizerLegacy, DEBUG_TYPE,
+ "SI Load Store Optimizer", false, false)
-char SILoadStoreOptimizer::ID = 0;
+char SILoadStoreOptimizerLegacy::ID = 0;
-char &llvm::SILoadStoreOptimizerID = SILoadStoreOptimizer::ID;
+char &llvm::SILoadStoreOptimizerLegacyID = SILoadStoreOptimizerLegacy::ID;
-FunctionPass *llvm::createSILoadStoreOptimizerPass() {
- return new SILoadStoreOptimizer();
+FunctionPass *llvm::createSILoadStoreOptimizerLegacyPass() {
+ return new SILoadStoreOptimizerLegacy();
}
static void addDefsUsesToList(const MachineInstr &MI,
@@ -2522,10 +2527,15 @@ SILoadStoreOptimizer::optimizeInstsWithSameBaseAddr(
return Modified;
}
-bool SILoadStoreOptimizer::runOnMachineFunction(MachineFunction &MF) {
+bool SILoadStoreOptimizerLegacy::runOnMachineFunction(MachineFunction &MF) {
if (skipFunction(MF.getFunction()))
return false;
+ return SILoadStoreOptimizer(
+ &getAnalysis<AAResultsWrapperPass>().getAAResults())
+ .run(MF);
+}
+bool SILoadStoreOptimizer::run(MachineFunction &MF) {
STM = &MF.getSubtarget<GCNSubtarget>();
if (!STM->loadStoreOptEnabled())
return false;
@@ -2534,7 +2544,6 @@ bool SILoadStoreOptimizer::runOnMachineFunction(MachineFunction &MF) {
TRI = &TII->getRegisterInfo();
MRI = &MF.getRegInfo();
- AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
LLVM_DEBUG(dbgs() << "Running SILoadStoreOptimizer\n");
@@ -2571,3 +2580,18 @@ bool SILoadStoreOptimizer::runOnMachineFunction(MachineFunction &MF) {
return Modified;
}
+
+PreservedAnalyses
+SILoadStoreOptimizerPass::run(MachineFunction &MF,
+ MachineFunctionAnalysisManager &MFAM) {
+ auto &FAM = MFAM.getResult<FunctionAnalysisManagerMachineFunctionProxy>(MF)
+ .getManager();
+ AAResults &AA = FAM.getResult<AAManager>(MF.getFunction());
+ bool Changed = SILoadStoreOptimizer(&AA).run(MF);
+ if (!Changed) {
+ return PreservedAnalyses::all();
+ }
+ PreservedAnalyses PA = getMachineFunctionPassPreservedAnalyses();
+ PA.preserveSet<CFGAnalyses>();
+ return PA;
+}
diff --git a/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.h b/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.h
new file mode 100644
index 00000000000000..fca5ad6924123d
--- /dev/null
+++ b/llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.h
@@ -0,0 +1,30 @@
+//===--- SILoadStoreOptimizer.h -------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_AMDGPU_SILOADSTOREOPTIMIZER_H
+#define LLVM_LIB_TARGET_AMDGPU_SILOADSTOREOPTIMIZER_H
+
+#include "llvm/CodeGen/MachinePassManager.h"
+
+namespace llvm {
+
+class SILoadStoreOptimizerPass
+ : public PassInfoMixin<SILoadStoreOptimizerPass> {
+public:
+ PreservedAnalyses run(MachineFunction &MF,
+ MachineFunctionAnalysisManager &MFAM);
+
+ MachineFunctionProperties getRequiredProperties() {
+ return MachineFunctionProperties().set(
+ MachineFunctionProperties::Property::IsSSA);
+ }
+};
+
+} // namespace llvm
+
+#endif // LLVM_LIB_TARGET_AMDGPU_SILOADSTOREOPTIMIZER_H
\ No newline at end of file
diff --git a/llvm/test/CodeGen/AMDGPU/load-store-opt-dlc.mir b/llvm/test/CodeGen/AMDGPU/load-store-opt-dlc.mir
index f4cdedf9cf6eb8..9295bd59621039 100644
--- a/llvm/test/CodeGen/AMDGPU/load-store-opt-dlc.mir
+++ b/llvm/test/CodeGen/AMDGPU/load-store-opt-dlc.mir
@@ -1,4 +1,5 @@
# RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -run-pass=si-load-store-opt -verify-machineinstrs -o - %s | FileCheck %s
+# RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -passes=si-load-store-opt -verify-machineinstrs -o - %s | FileCheck %s
# The purpose of this test is to make sure we are combining relevant memory
# operations correctly with/without DLC bit.
diff --git a/llvm/test/CodeGen/AMDGPU/load-store-opt-scc.mir b/llvm/test/CodeGen/AMDGPU/load-store-opt-scc.mir
index c4e131b90deb48..c0cc3e9f4edd7f 100644
--- a/llvm/test/CodeGen/AMDGPU/load-store-opt-scc.mir
+++ b/llvm/test/CodeGen/AMDGPU/load-store-opt-scc.mir
@@ -1,4 +1,5 @@
# RUN: llc -mtriple=amdgcn -mcpu=gfx90a -run-pass=si-load-store-opt -verify-machineinstrs -o - %s | FileCheck %s
+# RUN: llc -mtriple=amdgcn -mcpu=gfx90a -passes=si-load-store-opt -verify-machineinstrs -o - %s | FileCheck %s
# The purpose of this test is to make sure we are combining relevant memory
# operations correctly with/without SCC bit.
|
optimisan
force-pushed
the
port-si-load-store-opt-pass
branch
from
dfc06f0 to
8afb6eb
Compare
optimisan
commented
Aug 28, 2024
| PreservedAnalyses | ||
| SILoadStoreOptimizerPass::run(MachineFunction &MF, | ||
| MachineFunctionAnalysisManager &MFAM) { | ||
| MFPropsModifier _(*this, MF); |
There was a problem hiding this comment.
Why is MFPropsModifier not used in all passes that have getRequiredProperties and is this required?
There was a problem hiding this comment.
It should be required, and many passes are failing to report all the required properties. This is necessary to trigger an appropriate verifier error when manually running mir passes
There was a problem hiding this comment.
Not all passes in CodeGen pipeline set them although some of them should. MFPropsModifier is new pass manager only, we should ensure PassName(...).run(...) also set properties correctly. If pass require some properties, just use it or do something manually.
There was a problem hiding this comment.
If MFPropsModifier is new pass only, it seems that legacy passes do not use this either.
Should I add this to the legacy pass as well? Which is:
bool GCNDPPCombineLegacy::runOnMachineFunction(MachineFunction &MF) {
if (skipFunction(MF.getFunction()))
return false;
MFPropsModifier _(*this, MF);
return GCNDPPCombine().run(MF);
}There was a problem hiding this comment.
There is no need to use them in legacy pass manager version. In legacy pass manager, getRequiredProperties etc. are virtual method and MachineFunctionProperties are handled by MachineFunctionPass(see MachineFunctionPass::doInitialization and MachineFunctionPass::runOnFunction).
arsenm
reviewed
Aug 28, 2024
| PreservedAnalyses | ||
| SILoadStoreOptimizerPass::run(MachineFunction &MF, | ||
| MachineFunctionAnalysisManager &MFAM) { | ||
| MFPropsModifier _(*this, MF); |
There was a problem hiding this comment.
It should be required, and many passes are failing to report all the required properties. This is necessary to trigger an appropriate verifier error when manually running mir passes
cdevadas
reviewed
Aug 28, 2024
| @@ -1,4 +1,5 @@ | |||
| # RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -run-pass=si-load-store-opt -verify-machineinstrs -o - %s | FileCheck %s | |||
| # RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -passes=si-load-store-opt -verify-machineinstrs -o - %s | FileCheck %s | |||
There was a problem hiding this comment.
Verify is default when using new pass manager.
There was a problem hiding this comment.
I think the default should be pre and post verify the MIR, and the flag should be interpreted as run after each pass
There was a problem hiding this comment.
@paperchalice I wonder why the verify is default when using NPM? Shouldn't the behavior be consistent with the legacy PM, at least this early stage?
There was a problem hiding this comment.
Because verify is no longer a part in pass pipeline when using new pass manager. New pass manager introduces PassInstrumentation, verify now is now part of StandardInstrumentations::Verify which support both kinds of IRs. This can be disabled by option --disable-verify, but the doc string is not updated...
There was a problem hiding this comment.
we should match opt's -disable-verify and -verify-each
There was a problem hiding this comment.
we should match
opt's-disable-verifyand-verify-each
Now in #106665.
arsenm
reviewed
Aug 29, 2024
| if (MF.getFunction().hasOptNone()) | ||
| return PreservedAnalyses::all(); | ||
|
|
||
| MFPropsModifier _(*this, MF); |
There was a problem hiding this comment.
This should probably be before the optnone skip
There was a problem hiding this comment.
optnone is like skipFunction and properties check is a part of the pass, so I thought we should skip the check as well.
There was a problem hiding this comment.
Yes, but in terms of property changes I would expect the pass to always behave the same way
There was a problem hiding this comment.
Right, got it. And the legacy pass manager works the other way, so I'll change that.
arsenm
approved these changes
Aug 29, 2024
optimisan
force-pushed
the
port-si-load-store-opt-pass
branch
from
95b994f to
5e546a6
Compare
optimisan
force-pushed
the
port-si-load-store-opt-pass
branch
from
5e546a6 to
6445a69
Compare
cdevadas
approved these changes
Aug 29, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/3361 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
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.
No description provided.