Move SI Lower Control Flow Up - #159557
Conversation
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- llvm/lib/Target/AMDGPU/AMDGPU.h llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp llvm/lib/Target/AMDGPU/SIInstrInfo.cpp llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp b/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
index 9fa00d476..3828b0a65 100644
--- a/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
+++ b/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
@@ -159,9 +159,9 @@ public:
// Should preserve the same set that TwoAddressInstructions does.
AU.addPreserved<MachineDominatorTreeWrapperPass>();
AU.addPreserved<MachinePostDominatorTreeWrapperPass>();
- //AU.addPreserved<SlotIndexesWrapperPass>();
- //AU.addPreserved<LiveIntervalsWrapperPass>();
- //AU.addPreserved<LiveVariablesWrapperPass>();
+ // AU.addPreserved<SlotIndexesWrapperPass>();
+ // AU.addPreserved<LiveIntervalsWrapperPass>();
+ // AU.addPreserved<LiveVariablesWrapperPass>();
AU.addPreserved<MachineBlockFrequencyInfoWrapperPass>();
AU.addPreserved<MachineRegisterClassInfoWrapperPass>();
MachineFunctionPass::getAnalysisUsage(AU);
@@ -907,9 +907,9 @@ SILowerControlFlowPass::run(MachineFunction &MF,
auto PA = getMachineFunctionPassPreservedAnalyses();
PA.preserve<MachineDominatorTreeAnalysis>();
PA.preserve<MachinePostDominatorTreeAnalysis>();
- //PA.preserve<SlotIndexesAnalysis>();
- //PA.preserve<LiveIntervalsAnalysis>();
- //PA.preserve<LiveVariablesAnalysis>();
+ // PA.preserve<SlotIndexesAnalysis>();
+ // PA.preserve<LiveIntervalsAnalysis>();
+ // PA.preserve<LiveVariablesAnalysis>();
PA.preserve<MachineBlockFrequencyAnalysis>();
return PA;
}
|
|
|
||
| bool phi_seen = false; | ||
| MachineBasicBlock::iterator first_phi; | ||
| for (first_phi = MBB.begin(); first_phi != MBB.end(); first_phi++) |
There was a problem hiding this comment.
bool phi_seen = !MBB.phis().empty() ?
or even:
if (!MBB.phis().empty()) first_phi = MBB.phis().begin();
| break; | ||
| } | ||
|
|
||
| if (!phi_seen) { |
There was a problem hiding this comment.
Not necessary as SILowerControlFlow always insert at block begin.
MachineBasicBlock::iterator Start = MBB.begin();
Register SaveReg = MRI->createVirtualRegister(BoolRC);
MachineInstr *OrSaveExec =
BuildMI(MBB, Start, DL, TII->get(LMC.OrSaveExecOpc), SaveReg)
.add(MI.getOperand(1)); // Saved EXEC
move_ins_before_phis(*OrSaveExec);
| if (branch_MI.isBranch() && TII.getBranchDestBlock(branch_MI) == &succ_MBB) | ||
| return ++Epilog_Iterator(branch_MI.getIterator()); | ||
|
|
||
| llvm_unreachable("There should always be a branch to succ_MBB."); |
There was a problem hiding this comment.
What is about fall through?
bb.1: successors: %bb.2(0x80000000)
%64:vgpr_32 = V_MOV_B32_e32 100, implicit $exec
bb.2: successors: %bb.5(0x40000000), %bb.3(0x40000000)
There was a problem hiding this comment.
It looked like the IR wasn't allowed to have fall-throughs at this stage, because I didn't see any and I did see unconditional-branch-to-next. Are fall-throughs instead allowed and just not common at this stage?
There was a problem hiding this comment.
The fall-throughs are not prohibited and I have seen enough valid MIR in SSA with fall-through CF.
There was a problem hiding this comment.
Hmm, okay, I can normalize it by adding unconditional branches to next.
| @@ -323,6 +332,8 @@ void SILowerControlFlow::emitElse(MachineInstr &MI) { | |||
| if (LV) | |||
| LV->replaceKillInstruction(SrcReg, MI, *OrSaveExec); | |||
|
|
|||
| move_ins_before_phis(*OrSaveExec); | |||
There was a problem hiding this comment.
EXEC restoring instructions happen not only in ELSE
There was a problem hiding this comment.
Yes, but the other ones are at the end of the blocks, not before the PHIs, right?
There was a problem hiding this comment.
If we lower SI_END_CF and don't split block we are going to have Exec = Exec OR Masked in the beginning or middle.
There was a problem hiding this comment.
Hmm, I'll take a look.
There was a problem hiding this comment.
@alex-t, I am not seeing the problem. The surrounding code makes clear that the insertion can only ever happen at block begin. I did not change those lines. Are you sure there is an issue here?
MachineBasicBlock::iterator Start = MBB.begin();
// This must be inserted before phis and any spill code inserted before the
// else.
Register SaveReg = MRI->createVirtualRegister(BoolRC);
MachineInstr *OrSaveExec =
BuildMI(MBB, Start, DL, TII->get(LMC.OrSaveExecOpc), SaveReg)
.add(MI.getOperand(1)); // Saved EXEC
if (LV)
LV->replaceKillInstruction(SrcReg, MI, *OrSaveExec);
moveInsBeforePhis(*OrSaveExec);
| MachineInstr *cloned_MI = MF.CloneMachineInstr(&MI); | ||
| cloned_MI->getOperand(0).setReg(cloned_reg); | ||
| phi.addReg(cloned_reg).addMBB(pred_MBB); | ||
| pred_MBB->insertAfterBundle(branch_MI.getIterator(), cloned_MI); |
There was a problem hiding this comment.
You need to make sure that there is no Machine Verifier between SILowerControlFlow and SIRestoreNormalEpilog
Also, check if unusual placement instructions after the branch does not break SILowerControlFlow logic itself
There was a problem hiding this comment.
There must not be a machine verifier or the testcase would have broken, I think.
4d4f71a to
07f9460
Compare
07f9460 to
734b863
Compare
734b863 to
28c01b0
Compare
|
@llvm/pr-subscribers-backend-amdgpu Author: Patrick Simmons (linuxrocks123) ChangesStill needs tests and testing. Also needs camel case. If the snake case bothers anyone, I'll fix it now. Full diff: https://github.com/llvm/llvm-project/pull/159557.diff 7 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPU.h b/llvm/lib/Target/AMDGPU/AMDGPU.h
index 67042b700c047..bff430f0f1967 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPU.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPU.h
@@ -248,6 +248,9 @@ extern char &AMDGPUPreloadKernArgPrologLegacyID;
void initializeAMDGPUPreloadKernelArgumentsLegacyPass(PassRegistry &);
extern char &AMDGPUPreloadKernelArgumentsLegacyID;
+void initializeSIRestoreNormalEpilogLegacyPass(PassRegistry &);
+extern char &SIRestoreNormalEpilogLegacyID;
+
// Passes common to R600 and SI
FunctionPass *createAMDGPUPromoteAlloca();
void initializeAMDGPUPromoteAllocaPass(PassRegistry&);
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
index b87b54ffc4f12..5287ea30a1c1b 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -603,6 +603,7 @@ extern "C" LLVM_ABI LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAMDGPUTarget() {
initializeSIOptimizeExecMaskingLegacyPass(*PR);
initializeSIPreAllocateWWMRegsLegacyPass(*PR);
initializeSIFormMemoryClausesLegacyPass(*PR);
+ initializeSIRestoreNormalEpilogLegacyPass(*PR);
initializeSIPostRABundlerLegacyPass(*PR);
initializeGCNCreateVOPDLegacyPass(*PR);
initializeAMDGPUUnifyDivergentExitNodesPass(*PR);
@@ -1573,7 +1574,7 @@ void GCNPassConfig::addFastRegAlloc() {
// This must be run immediately after phi elimination and before
// TwoAddressInstructions, otherwise the processing of the tied operand of
// SI_ELSE will introduce a copy of the tied operand source after the else.
- insertPass(&PHIEliminationID, &SILowerControlFlowLegacyID);
+ //insertPass(&PHIEliminationID, &SILowerControlFlowLegacyID);
insertPass(&TwoAddressInstructionPassID, &SIWholeQuadModeID);
@@ -1596,13 +1597,17 @@ void GCNPassConfig::addOptimizedRegAlloc() {
if (OptVGPRLiveRange)
insertPass(&LiveVariablesID, &SIOptimizeVGPRLiveRangeLegacyID);
+ insertPass(&SIOptimizeVGPRLiveRangeLegacyID, &SILowerControlFlowLegacyID);
+
// This must be run immediately after phi elimination and before
// TwoAddressInstructions, otherwise the processing of the tied operand of
// SI_ELSE will introduce a copy of the tied operand source after the else.
- insertPass(&PHIEliminationID, &SILowerControlFlowLegacyID);
+ //insertPass(&PHIEliminationID, &SILowerControlFlowLegacyID);
if (EnableRewritePartialRegUses)
insertPass(&RenameIndependentSubregsID, &GCNRewritePartialRegUsesID);
+
+ insertPass(&RenameIndependentSubregsID,&SIRestoreNormalEpilogLegacyID);
if (isPassEnabled(EnablePreRAOptimizations))
insertPass(&MachineSchedulerID, &GCNPreRAOptimizationsID);
@@ -2259,7 +2264,7 @@ void AMDGPUCodeGenPassBuilder::addOptimizedRegAlloc(
// This must be run immediately after phi elimination and before
// TwoAddressInstructions, otherwise the processing of the tied operand of
// SI_ELSE will introduce a copy of the tied operand source after the else.
- insertPass<PHIEliminationPass>(SILowerControlFlowPass());
+ //insertPass<PHIEliminationPass>(SILowerControlFlowPass());
if (EnableRewritePartialRegUses)
insertPass<RenameIndependentSubregsPass>(GCNRewritePartialRegUsesPass());
diff --git a/llvm/lib/Target/AMDGPU/CMakeLists.txt b/llvm/lib/Target/AMDGPU/CMakeLists.txt
index a1e0e5293c706..d035f7aea3298 100644
--- a/llvm/lib/Target/AMDGPU/CMakeLists.txt
+++ b/llvm/lib/Target/AMDGPU/CMakeLists.txt
@@ -185,6 +185,7 @@ add_llvm_target(AMDGPUCodeGen
SIPreEmitPeephole.cpp
SIProgramInfo.cpp
SIRegisterInfo.cpp
+ SIRestoreNormalEpilog.cpp
SIShrinkInstructions.cpp
SIWholeQuadMode.cpp
diff --git a/llvm/lib/Target/AMDGPU/SICustomBranchBundles.h b/llvm/lib/Target/AMDGPU/SICustomBranchBundles.h
new file mode 100644
index 0000000000000..03ee6a251f391
--- /dev/null
+++ b/llvm/lib/Target/AMDGPU/SICustomBranchBundles.h
@@ -0,0 +1,261 @@
+#pragma once
+
+#include "GCNSubtarget.h"
+#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/MachineBasicBlock.h"
+#include "llvm/CodeGen/MachineInstr.h"
+#include "llvm/Support/ErrorHandling.h"
+
+#include "SIInstrInfo.h"
+
+#include <cassert>
+#include <unordered_set>
+
+using namespace llvm;
+
+using std::unordered_set;
+using std::vector;
+
+static inline MachineInstr &getBranchWithDest(MachineBasicBlock &BranchingMBB,
+ MachineBasicBlock &DestMBB) {
+ auto &TII =
+ *BranchingMBB.getParent()->getSubtarget<GCNSubtarget>().getInstrInfo();
+ for (MachineInstr &BranchMI : reverse(BranchingMBB.instrs()))
+ if (BranchMI.isBranch() && TII.getBranchDestBlock(BranchMI) == &DestMBB)
+ return BranchMI;
+
+ llvm_unreachable("Don't call this if there's no branch to the destination.");
+}
+
+static inline void moveInsBeforePhis(MachineInstr &MI) {
+ MachineBasicBlock &MBB = *MI.getParent();
+ MachineFunction &MF = *MBB.getParent();
+ auto &TII = *MF.getSubtarget<GCNSubtarget>().getInstrInfo();
+ auto &MRI = MF.getRegInfo();
+
+ bool PhiSeen = false;
+ MachineBasicBlock::iterator FirstPhi;
+ for (FirstPhi = MBB.begin(); FirstPhi != MBB.end(); FirstPhi++)
+ if (FirstPhi->getOpcode() == AMDGPU::PHI) {
+ PhiSeen = true;
+ break;
+ }
+
+ if (!PhiSeen) {
+ MI.removeFromParent();
+ MBB.insert(MBB.begin(), &MI);
+ } else {
+ auto Phi = BuildMI(MBB, FirstPhi, MI.getDebugLoc(), TII.get(AMDGPU::PHI),
+ MI.getOperand(0).getReg());
+ for (auto *PredMBB : MBB.predecessors()) {
+ Register ClonedReg = MRI.cloneVirtualRegister(MI.getOperand(0).getReg());
+ MachineInstr &BranchMI = getBranchWithDest(*PredMBB, MBB);
+ MachineInstr *ClonedMI = MF.CloneMachineInstr(&MI);
+ ClonedMI->getOperand(0).setReg(ClonedReg);
+ Phi.addReg(ClonedReg).addMBB(PredMBB);
+ PredMBB->insertAfterBundle(BranchMI.getIterator(), ClonedMI);
+ ClonedMI->bundleWithPred();
+ }
+ MI.eraseFromParent();
+ }
+}
+
+struct EpilogIterator {
+ MachineBasicBlock::instr_iterator InternalIt;
+ EpilogIterator(MachineBasicBlock::instr_iterator I) : InternalIt(I) {}
+
+ bool operator==(const EpilogIterator &Other) {
+ return InternalIt == Other.InternalIt;
+ }
+ bool isEnd() { return InternalIt.isEnd(); }
+ MachineInstr &operator*() { return *InternalIt; }
+ MachineBasicBlock::instr_iterator operator->() { return InternalIt; }
+ EpilogIterator &operator++() {
+ ++InternalIt;
+ if (!InternalIt.isEnd() && InternalIt->isBranch())
+ InternalIt = InternalIt->getParent()->instr_end();
+ return *this;
+ }
+ EpilogIterator operator++(int Ignored) {
+ EpilogIterator ToReturn = *this;
+ ++*this;
+ return ToReturn;
+ }
+};
+
+static inline EpilogIterator getEpilogForSuccessor(MachineBasicBlock &PredMBB,
+ MachineBasicBlock &SuccMBB) {
+ MachineFunction &MF = *PredMBB.getParent();
+ auto &TII = *MF.getSubtarget<GCNSubtarget>().getInstrInfo();
+
+ for (MachineInstr &BranchMI : reverse(PredMBB.instrs()))
+ if (BranchMI.isBranch() && TII.getBranchDestBlock(BranchMI) == &SuccMBB)
+ return ++EpilogIterator(BranchMI.getIterator());
+
+ llvm_unreachable("There should always be a branch to succ_MBB.");
+}
+
+static inline bool epilogsAreIdentical(const vector<MachineInstr *> Left,
+ const vector<MachineInstr *> Right,
+ const MachineBasicBlock &SuccMBB) {
+ if (Left.size() != Right.size())
+ return false;
+
+ for (unsigned I = 0; I < Left.size(); I++)
+ if (!Left[I]->isIdenticalTo(*Right[I]))
+ return false;
+ return true;
+}
+
+static inline void moveBody(vector<MachineInstr *> &Body,
+ MachineBasicBlock &DestMBB) {
+ for (auto RevIt = Body.rbegin(); RevIt != Body.rend(); RevIt++) {
+ MachineInstr &BodyIns = **RevIt;
+ BodyIns.removeFromBundle();
+ DestMBB.insert(DestMBB.begin(), &BodyIns);
+ }
+}
+
+static inline void normalizeIrPostPhiElimination(MachineFunction &MF) {
+ auto &TII = *MF.getSubtarget<GCNSubtarget>().getInstrInfo();
+
+ struct CFGRewriteEntry {
+ unordered_set<MachineBasicBlock *> PredMBBs;
+ MachineBasicBlock *SuccMBB;
+ vector<MachineInstr *> Body;
+ };
+
+ vector<CFGRewriteEntry> CfgRewriteEntries;
+ for (MachineBasicBlock &MBB : MF) {
+ CFGRewriteEntry ToInsert = {{}, &MBB, {}};
+ for (MachineBasicBlock *PredMBB : MBB.predecessors()) {
+ EpilogIterator EpIt = getEpilogForSuccessor(*PredMBB, MBB);
+
+ vector<MachineInstr *> Epilog;
+ while (!EpIt.isEnd())
+ Epilog.push_back(&*EpIt++);
+
+ if (!epilogsAreIdentical(ToInsert.Body, Epilog, MBB)) {
+ if (ToInsert.PredMBBs.size() && ToInsert.Body.size()) {
+ // Potentially, we need to insert a new entry. But first see if we
+ // can find an existing entry with the same epilog.
+ bool ExistingEntryFound = false;
+ for (auto RevIt = CfgRewriteEntries.rbegin();
+ RevIt != CfgRewriteEntries.rend() && RevIt->SuccMBB == &MBB;
+ RevIt++)
+ if (epilogsAreIdentical(RevIt->Body, Epilog, MBB)) {
+ RevIt->PredMBBs.insert(PredMBB);
+ ExistingEntryFound = true;
+ break;
+ }
+
+ if (!ExistingEntryFound)
+ CfgRewriteEntries.push_back(ToInsert);
+ }
+ ToInsert.PredMBBs.clear();
+ ToInsert.Body = Epilog;
+ }
+
+ ToInsert.PredMBBs.insert(PredMBB);
+ }
+
+ // Handle the last potential rewrite entry. Lower instead of journaling a
+ // rewrite entry if all predecessor MBBs are in this single entry.
+ if (ToInsert.PredMBBs.size() == MBB.pred_size()) {
+ moveBody(ToInsert.Body, MBB);
+ for (MachineBasicBlock *PredMBB : ToInsert.PredMBBs) {
+ // Delete instructions that were lowered from epilog
+ MachineInstr &BranchIns =
+ getBranchWithDest(*PredMBB, *ToInsert.SuccMBB);
+ auto EpilogIt = ++EpilogIterator(BranchIns.getIterator());
+ while (!EpilogIt.isEnd())
+ EpilogIt++->eraseFromBundle();
+ }
+
+ } else if (ToInsert.Body.size())
+ CfgRewriteEntries.push_back(ToInsert);
+ }
+
+ // Perform the journaled rewrites.
+ for (auto &Entry : CfgRewriteEntries) {
+ MachineBasicBlock *MezzanineMBB = MF.CreateMachineBasicBlock();
+ MF.insert(MF.end(), MezzanineMBB);
+
+ // Deal with mezzanine to successor succession.
+ BuildMI(MezzanineMBB, DebugLoc(), TII.get(AMDGPU::S_BRANCH))
+ .addMBB(Entry.SuccMBB);
+ MezzanineMBB->addSuccessor(Entry.SuccMBB);
+
+ // Move instructions to mezzanine block.
+ moveBody(Entry.Body, *MezzanineMBB);
+
+ for (MachineBasicBlock *PredMBB : Entry.PredMBBs) {
+ // Deal with predecessor to mezzanine succession.
+ MachineInstr &BranchIns = getBranchWithDest(*PredMBB, *Entry.SuccMBB);
+ assert(BranchIns.getOperand(0).isMBB() && "Branch instruction isn't.");
+ BranchIns.getOperand(0).setMBB(MezzanineMBB);
+ PredMBB->replaceSuccessor(Entry.SuccMBB, MezzanineMBB);
+
+ // Delete instructions that were lowered from epilog
+ auto EpilogIt = ++EpilogIterator(BranchIns.getIterator());
+ while (!EpilogIt.isEnd())
+ EpilogIt++->eraseFromBundle();
+ }
+ }
+}
+
+namespace std {
+template <> struct hash<Register> {
+ std::size_t operator()(const Register &R) const {
+ return hash<unsigned>()(R);
+ }
+};
+} // namespace std
+
+static inline void hoistUnrelatedCopies(MachineFunction &MF) {
+ for (MachineBasicBlock &MBB : MF)
+ for (MachineInstr &BranchMI : MBB) {
+ if (!BranchMI.isBranch())
+ continue;
+
+ unordered_set<Register> RelatedCopySources;
+ EpilogIterator EpilogIt = BranchMI.getIterator();
+ EpilogIterator CopyMoveIt = ++EpilogIt;
+ while (!EpilogIt.isEnd()) {
+ if (EpilogIt->getOpcode() != AMDGPU::COPY)
+ RelatedCopySources.insert(EpilogIt->getOperand(0).getReg());
+ ++EpilogIt;
+ }
+
+ while (!CopyMoveIt.isEnd()) {
+ EpilogIterator Next = CopyMoveIt;
+ ++Next;
+ if ((CopyMoveIt->getOpcode() == AMDGPU::COPY &&
+ !RelatedCopySources.count(CopyMoveIt->getOperand(1).getReg())) ||
+ CopyMoveIt->getOpcode() == AMDGPU::IMPLICIT_DEF) {
+ MachineInstr &MIToMove = *CopyMoveIt;
+ MIToMove.removeFromBundle();
+ MBB.insert(BranchMI.getIterator(), &MIToMove);
+ }
+
+ CopyMoveIt = Next;
+ }
+ }
+}
+
+static inline bool makeEverySuccessorBeBranchTarget(MachineFunction &MF) {
+ bool Changed = false;
+ auto &TII = *MF.getSubtarget<GCNSubtarget>().getInstrInfo();
+ for (MachineBasicBlock &MBB : MF)
+ if (MBB.empty() ||
+ (!MBB.back().isUnconditionalBranch() && !MBB.back().isReturn())) {
+ MachineBasicBlock *LayoutSuccessor =
+ &*std::next(MachineFunction::iterator(MBB));
+ BuildMI(&MBB, DebugLoc(), TII.get(AMDGPU::S_BRANCH))
+ .addMBB(LayoutSuccessor);
+ Changed = true;
+ }
+
+ return Changed;
+}
diff --git a/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp b/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
index 8586d6c18b361..64d46c5e401de 100644
--- a/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
+++ b/llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp
@@ -48,6 +48,7 @@
/// %exec = S_OR_B64 %exec, %sgpr0 // Re-enable saved exec mask bits
//===----------------------------------------------------------------------===//
+#include "SICustomBranchBundles.h"
#include "SILowerControlFlow.h"
#include "AMDGPU.h"
#include "AMDGPULaneMaskUtils.h"
@@ -152,6 +153,14 @@ class SILowerControlFlowLegacy : public MachineFunctionPass {
return "SI Lower control flow pseudo instructions";
}
+ MachineFunctionProperties getRequiredProperties() const override {
+ return MachineFunctionProperties().setIsSSA();
+ }
+
+ MachineFunctionProperties getClearedProperties() const override {
+ return MachineFunctionProperties().setNoPHIs();
+ }
+
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.addUsedIfAvailable<LiveIntervalsWrapperPass>();
// Should preserve the same set that TwoAddressInstructions does.
@@ -322,6 +331,8 @@ void SILowerControlFlow::emitElse(MachineInstr &MI) {
if (LV)
LV->replaceKillInstruction(SrcReg, MI, *OrSaveExec);
+ moveInsBeforePhis(*OrSaveExec);
+
MachineBasicBlock *DestBB = MI.getOperand(2).getMBB();
MachineBasicBlock::iterator ElsePt(MI);
@@ -789,7 +800,7 @@ bool SILowerControlFlow::run(MachineFunction &MF) {
}
}
- bool Changed = false;
+ bool Changed = makeEverySuccessorBeBranchTarget(MF);
MachineFunction::iterator NextBB;
for (MachineFunction::iterator BI = MF.begin();
BI != MF.end(); BI = NextBB) {
@@ -839,6 +850,12 @@ bool SILowerControlFlow::run(MachineFunction &MF) {
LoweredIf.clear();
KillBlocks.clear();
+ if (Changed)
+ for (MachineBasicBlock &MBB : MF)
+ for (MachineInstr &MI : MBB)
+ if (MI.isBundled())
+ MI.unbundleFromSucc();
+
return Changed;
}
diff --git a/llvm/lib/Target/AMDGPU/SILowerControlFlow.h b/llvm/lib/Target/AMDGPU/SILowerControlFlow.h
index 23803c679c246..0f4df79952999 100644
--- a/llvm/lib/Target/AMDGPU/SILowerControlFlow.h
+++ b/llvm/lib/Target/AMDGPU/SILowerControlFlow.h
@@ -16,6 +16,14 @@ class SILowerControlFlowPass : public PassInfoMixin<SILowerControlFlowPass> {
public:
PreservedAnalyses run(MachineFunction &MF,
MachineFunctionAnalysisManager &MFAM);
+
+ MachineFunctionProperties getRequiredProperties() const {
+ return MachineFunctionProperties().setIsSSA();
+ }
+
+ MachineFunctionProperties getClearedProperties() const {
+ return MachineFunctionProperties().setNoPHIs();
+ }
};
} // namespace llvm
diff --git a/llvm/lib/Target/AMDGPU/SIRestoreNormalEpilog.cpp b/llvm/lib/Target/AMDGPU/SIRestoreNormalEpilog.cpp
new file mode 100644
index 0000000000000..d6d02c940731c
--- /dev/null
+++ b/llvm/lib/Target/AMDGPU/SIRestoreNormalEpilog.cpp
@@ -0,0 +1,46 @@
+#include "SICustomBranchBundles.h"
+#include "AMDGPU.h"
+#include "GCNSubtarget.h"
+#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/CodeGen/LiveIntervals.h"
+#include "llvm/CodeGen/LiveVariables.h"
+#include "llvm/CodeGen/MachineDominators.h"
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/MachinePostDominators.h"
+#include "llvm/Target/TargetMachine.h"
+
+#define DEBUG_TYPE "si-restore-normal-epilog"
+
+namespace
+{
+
+class SIRestoreNormalEpilogLegacy : public MachineFunctionPass {
+public:
+ static char ID;
+
+ SIRestoreNormalEpilogLegacy() : MachineFunctionPass(ID) {}
+
+ bool runOnMachineFunction(MachineFunction &MF) override {
+ hoistUnrelatedCopies(MF);
+ normalizeIrPostPhiElimination(MF);
+ return true;
+ }
+
+ StringRef getPassName() const override {
+ return "SI Restore Normal Epilog Post PHI Elimination";
+ }
+
+ MachineFunctionProperties getRequiredProperties() const override {
+ return MachineFunctionProperties().setNoPHIs();
+ }
+
+};
+
+} // namespace
+
+INITIALIZE_PASS(SIRestoreNormalEpilogLegacy, DEBUG_TYPE,
+ "SI restore normal epilog", false, false)
+
+char SIRestoreNormalEpilogLegacy::ID;
+char &llvm::SIRestoreNormalEpilogLegacyID = SIRestoreNormalEpilogLegacy::ID;
|
|
@alex-t I've completed all action items for this, but there are some comments to which I've responded where you have not replied. Please let me know your thoughts. |
28c01b0 to
b5421a2
Compare
|
This needs a proper description and motivation - what does the patch do and why? |
|
@linuxrocks123 , please help with description and motivation @alex-t , please review |
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
This is quite a lot of code with no comments, description, or tests. l also recall having conversations about how to not use bundles for this
| @@ -0,0 +1,261 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
llvm does't use pragma once. Also missing file header.
There's also way too much code in the header, sink everything to the implementation file
There was a problem hiding this comment.
@arsenm will change #pragma once before merge, simultaneous with changing to camelCase. This code will need to be used by multiple passes, so it belongs in a header.
| using std::unordered_set; | ||
| using std::vector; |
There was a problem hiding this comment.
| using std::unordered_set; | |
| using std::vector; |
| @@ -0,0 +1,46 @@ | |||
| #include "SICustomBranchBundles.h" | |||
| if (Changed) | ||
| for (MachineBasicBlock &MBB : MF) | ||
| for (MachineInstr &MI : MBB) | ||
| if (MI.isBundled()) | ||
| MI.unbundleFromSucc(); |
There was a problem hiding this comment.
You can't just go through and strip all bundles
There was a problem hiding this comment.
@arsenm I think you can because someone told me they are otherwise unused at this level.
| template <> struct hash<Register> { | ||
| std::size_t operator()(const Register &R) const { | ||
| return hash<unsigned>()(R); | ||
| } |
There was a problem hiding this comment.
Not supposed to define things in namespace std
There was a problem hiding this comment.
@arsenm that is not correct: you are allowed to specialize std::hash. If you could not do that, it would not be possible to use unordered_map or unordered_set with anything except primitive and standard library types.
https://en.cppreference.com/w/cpp/language/extending_std.html
| #include "SIInstrInfo.h" | ||
|
|
||
| #include <cassert> | ||
| #include <unordered_set> |
There was a problem hiding this comment.
Probably should be using the llvm set and map types
There was a problem hiding this comment.
I know LLVM likes its private pseudo-STL, but I like the fact that STL types are transparent when printing under GDB. There are also a few footguns in the LLVM ADT library, such as ary[x] = ary[y]; being undefined behavior in DenseMap, which make me want to avoid using those types. We can always change before merge after everything else is finalized if we want to, because I won't care at that point since I don't have to step through the code anymore. I have an automated script that can do that. We'll want to manually verify none of the footguns are loaded if we use it.
| llvm_unreachable("Don't call this if there's no branch to the destination."); | ||
| } | ||
|
|
||
| static inline void moveInsBeforePhis(MachineInstr &MI) { |
There was a problem hiding this comment.
Instructions cannot be moved before phis
There was a problem hiding this comment.
EXEC-modifying instructions must happen prior to PHIs in block X. This function accomplishes that by moving them to the tails of all of the predecessors of X.
| ClonedMI->getOperand(0).setReg(ClonedReg); | ||
| Phi.addReg(ClonedReg).addMBB(PredMBB); | ||
| PredMBB->insertAfterBundle(BranchMI.getIterator(), ClonedMI); | ||
| ClonedMI->bundleWithPred(); |
There was a problem hiding this comment.
Didn't we discuss that you should not be trying to use bundles for this?
There was a problem hiding this comment.
We had a discussion which was unresolved. Part of the purpose of this PR is to look at the code and, if a non-bundle implementation is desired in its place, to decide what that implementation should be.
| namespace | ||
| { | ||
|
|
||
| class SIRestoreNormalEpilogLegacy : public MachineFunctionPass { |
There was a problem hiding this comment.
Also handle new pass manager. The "Legacy" in the name when it hasn't been ported is misleading
There was a problem hiding this comment.
Thanks, I'll do that.
| } | ||
| } | ||
|
|
||
| static inline bool makeEverySuccessorBeBranchTarget(MachineFunction &MF) { |
There was a problem hiding this comment.
Shouldn't be inserting new branches simply to make your other function simpler
There was a problem hiding this comment.
Why not? This is well before layout optimization.
There was a problem hiding this comment.
This is adding cost and complexity, you just need to directly handle all branch situations by using the control flow APIs
There was a problem hiding this comment.
@arsenm it seems to me that reusing the existing layout optimization code is reducing complexity. Writing more code to handle more cases would be adding complexity, right?
3d481d2 to
c442e62
Compare
What is the big picture here? Why do you want to "move SILowerControlFlow up" / run it in SSA form? Are you aware of the "wave transform" project (roughly, doing CFG structurization in MachineIR instead of IR) and is this related to that?
I find that statement hard to understand because conceptually PHI instructions do not "happen" where the instruction is placed, but on each of the incoming edges to the block they are placed in. But anyway, maybe understanding the high level motivation would help me to understand this part. |
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
As I have previously stated, this approach should not be pursued. There should be no intermediate bundling
|
@jayfoad this is part of a project to write a "perfect" register allocator that allocates directly out of SSA form. Since there will no longer be any "after SSA but before register allocation" IR state once this project is completed, it is necessary to move up certain passes that currently run before register allocation but after transformation out of SSA form so that those passes run in SSA form instead. |
|
@arsenm code that we do not primarily maintain runs between our own passes and our restoration of our IR to a normal format. Bundling provides a way of masking our IR differences to that code. If you know of an alternative to bundling that does not involve requiring intermediate passes to understand a new custom IR format, please let me know. Otherwise, we have to work with what LLVM IR provides for us, whether or not it's the ideal tool. |
The set of passes is maintained as a whole for the compiler
When we originally were discussing this, I was advocating for making fundamental MIR changes (i.e., introduce block arguments to replace phis). We do not just have to accept things as they are |
|
@arsenm that would be a lot of work, and I don't see a lot of potential gain, but the IR changes are encapsulated to accommodate that approach. Whoever implements that approach would need to modify all of the necessary target-independent upstream passes to understand a new Machine IR format used only by one target and may need to convince whoever maintains those passes that that new Machine IR format is beneficial to LLVM as a whole and should therefore be supported by target-independent passes. Once all of that has been done, I could update this PR to use the new format by updating SICustomBranchBundles.h to use whatever new IR feature has been added instead of bundles. Doing so should only require changing a few lines of code. |
|
@arsenm it may not be that hard to use a different IR format. I'm looking at what I have now again, and the bundles aren't surviving past a single pass right now anyway. We're not needing them to mask the weird IR from upstream passes. We just have loose instructions floating after conditional unconditional branches and most upstream passes seem cool with that. Of course, I'm looking at the code in that much detail because it's not working right after rebasing, so it may not be that rosy after all :) |
44b7345 to
f1500d9
Compare
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
I'm not sure why you keep updating this and it should be closed. The original approach was a dead end, and the latest version seems to be incomplete and have nothing in common with the starting point
This doesn't sound better, and this design should be pre-reviewed. And even so, this PR is old and cluttered and anything else should be a fresh PR |
|
The author prefers to keep working this ticket, please dont close it. |
f1500d9 to
58a805f
Compare
…nt VREGs in the hoisted instructions and merging them with PHI.
This is not up to the author. I am declaring this PR unreviewable; do not reopen it. Any further changes in this area need to be a new PR, not cluttered with a deeply flawed started point and this many comments |
|
I have moved this to linuxrocks123#2 to prevent further interference. |
7 participants
Still needs tests and testing. Also needs camel case. If the snake case bothers anyone, I'll fix it now.
Description: This PR creates and uses a custom Machine IR format necessary for doing certain transformations, including
SILowerControlFlow, while the code is still in SSA form. Our problem is that, conceptually, EXEC-modifying instructions must happen prior to thePHIinstructions in a block, but the code is not in SSA form if any instructions occur prior to thePHIs. This pass adds a header,SICustomBranchBundles.h, which addresses this issue by providing functions to transform our IR representation such that the "PHIinstructions are first" variant is maintained.One of these functions,
moveInsBeforePhis, conceptually moves an instruction "before thePHIs" in block X by copying it into a bundle with the branch in each of block X's predecessors that branches to block X. This function help transform the code into our custom SSA Machine IR.Some temporarily necessary scaffolding is necessary for two reasons. First, we don't have an SSA-form register allocator yet, so we are using the non-SSA one. Second, we have not converted all of our passes that must be converted to run in SSA form yet. As a form of scaffolding,
normalizeIrPostPhiElimination, transforms the code out of our custom SSA IR after allPHIinstructions have been eliminated. A new pass,SIRestoreNormalEpilog, uses that function to come out of our custom IR format.As another form of scaffolding,
SILowerControlFlowtemporarily unlinks all bundles in the source code after it usesmoveInsBeforePhis, so as not to confuse passes that have not been converted yet. This means that the IR is actually in a state where the instructions that should be bundled actually just occur in free-form after the branches they are attached to. This seems to work okay, but some unrelatedCOPYinstructions find their way into those free-form stanzas. So,hoistUnrelatedCopies, another piece of scaffolding, moves unnecessaryCOPYinstructions out of the free-form should-be-bundled-with-the-branch stanzas back to a point in the block prior to the first branch instruction.As additional passes are converted to SSA form, the scaffolding in
SILowerControlFlowwill be moved to the final "new SSA" pass which runs. Eventually, once we switch to an SSA register allocator, normalizing the IR afterPHIelimination will no longer be necessary, because the PHI Elimination pass will no longer be part of our pipeline, so thehoistUnrelatedCopiesandnormalizeIrPostPhiEliminationfunctions will no longer necessary and will be deleted. The same goes for theSIRestoreNormalEpilogpass.