<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 31, 2017 at 2:15 AM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Puyan Lotfi <<a href="mailto:puyan.lotfi.llvm@gmail.com">puyan.lotfi.llvm@gmail.com</a>> writes:<br>
> Thanks Justin!<br>
><br>
> I have an updated patch, addressing the provided feedback.<br>
<br>
</span>Looking better. I have a few more nitpicks below, and this could<br>
probably use a couple of simple tests for the basic functionality.<br>
<br>
I also ran it on a few random examples in test/AArch64/GlobalISel and<br>
noticed that it's a bit picky about exactly what kind of MIR it can<br>
handle right now, but it seems fine to improve that part once the<br>
initial patch goes in.<br>
<br>
> commit 38551ee3aa2e3723fc4dec970e9229<wbr>d9e0cdd807<br>
> Author: Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>><br>
> Date: Fri Oct 27 16:26:10 2017 -0700<br>
><br>
> mir-canon: First commit.<br>
><br>
> mir-canon (MIRCanonicalizerPass) is a pass designed to reorder<br>
> instructions and rename operands so that two similar programs will diff<br>
> more cleanly after bring run through mir-canon than they could<br>
> otherwise. This project is still a work in progress and there are ideas<br>
> still being discussed for improving diff quality.<br>
><br>
> diff --git a/include/llvm/<wbr>InitializePasses.h b/include/llvm/<wbr>InitializePasses.h<br>
> index 6b0e6acadad..18be1d250c8 100644<br>
> --- a/include/llvm/<wbr>InitializePasses.h<br>
> +++ b/include/llvm/<wbr>InitializePasses.h<br>
> @@ -378,6 +378,7 @@ void initializeWinEHPreparePass(<wbr>PassRegistry&);<br>
<div><div class="h5">> void initializeWriteBitcodePassPass<wbr>(PassRegistry&);<br>
> void initializeWriteThinLTOBitcodeP<wbr>ass(PassRegistry&);<br>
> void initializeXRayInstrumentationP<wbr>ass(PassRegistry&);<br>
> +void initializeMIRCanonicalizerPass<wbr>(PassRegistry &);<br>
><br>
> } // end namespace llvm<br>
><br>
> diff --git a/lib/CodeGen/CMakeLists.txt b/lib/CodeGen/CMakeLists.txt<br>
> index 7ec7fda4e44..2e364cd4794 100644<br>
> --- a/lib/CodeGen/CMakeLists.txt<br>
> +++ b/lib/CodeGen/CMakeLists.txt<br>
> @@ -113,6 +113,7 @@ add_llvm_library(LLVMCodeGen<br>
> RegisterPressure.cpp<br>
> RegisterScavenging.cpp<br>
> RenameIndependentSubregs.cpp<br>
> + MIRCanonicalizerPass.cpp<br>
> RegisterUsageInfo.cpp<br>
> RegUsageInfoCollector.cpp<br>
> RegUsageInfoPropagate.cpp<br>
> diff --git a/lib/CodeGen/CodeGen.cpp b/lib/CodeGen/CodeGen.cpp<br>
> index f4ccb4889d3..bfab865687e 100644<br>
> --- a/lib/CodeGen/CodeGen.cpp<br>
> +++ b/lib/CodeGen/CodeGen.cpp<br>
> @@ -99,6 +99,7 @@ void llvm::initializeCodeGen(<wbr>PassRegistry &Registry) {<br>
> initializeVirtRegRewriterPass(<wbr>Registry);<br>
> initializeWinEHPreparePass(<wbr>Registry);<br>
> initializeXRayInstrumentationP<wbr>ass(Registry);<br>
> + initializeMIRCanonicalizerPass<wbr>(Registry);<br>
> }<br>
><br>
> void LLVMInitializeCodeGen(<wbr>LLVMPassRegistryRef R) {<br>
> diff --git a/lib/CodeGen/<wbr>MIRCanonicalizerPass.cpp b/lib/CodeGen/<wbr>MIRCanonicalizerPass.cpp<br>
> new file mode 100644<br>
</div></div>> index 00000000000..b8c85e4ba37<br>
> --- /dev/null<br>
> +++ b/lib/CodeGen/<wbr>MIRCanonicalizerPass.cpp<br>
> @@ -0,0 +1,641 @@<br>
<span class="">> +//===-------------- MIRCanonicalizer.cpp - MIR Canonicalizer --------------===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
> +//<br>
</span>> +// The purpose of this pass is to establish a canonical operand naming so that<br>
> +// code compiled with slightly different IR passes can be diffed more<br>
> +// effectively than otherwise. This is done by renaming vregs in a given<br>
> +// LiveRange in a canonical way. This pass also does a pseudo-scheduling to move<br>
> +// defs closer to their use inorder to reduce diffs caused by slightly different<br>
> +// schedules. At the moment code coming out of this pass is meant to be diffed<br>
> +// manually with a diff tool like vimdiff or Beyond Compare and is not meant for<br>
> +// executable code generation, however it is possible that this may change in<br>
> +// the future.<br>
<br>
This is better, though I'd probably drop the last sentence - it seems a<br>
bit redundant with the point that we're reducing diffs here. It may be<br>
worth pointing out the basic usage (ie. `llc -run-pass=mir-canonicalizer`)<br>
<span class=""><br>
> +//<br>
> +// Reorders instructions canonically.<br>
> +// Renames virtual register operands canonically.<br>
> +// Strips certain MIR artifacts (optionally).<br>
</span>> +//<br>
> +//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
> +<br>
> +#include "llvm/ADT/PostOrderIterator.h"<br>
> +#include "llvm/ADT/STLExtras.h"<br>
> +#include "llvm/CodeGen/Passes.h"<br>
> +#include "llvm/CodeGen/<wbr>MachineFunctionPass.h"<br>
> +#include "llvm/CodeGen/<wbr>MachineInstrBuilder.h"<br>
> +#include "llvm/CodeGen/<wbr>MachineRegisterInfo.h"<br>
> +#include "llvm/Support/raw_ostream.h"<br>
> +#include "llvm/Target/TargetInstrInfo.<wbr>h"<br>
> +<br>
> +#include <queue><br>
<span class="">> +<br>
> +using namespace llvm;<br>
> +<br>
</span>> +namespace llvm {<br>
<div><div class="h5">> +extern char &MIRCanonicalizerID;<br>
> +} // namespace llvm<br>
> +<br>
> +#define DEBUG_TYPE "mir-canonicalizer"<br>
> +<br>
> +static cl::opt<unsigned><br>
> +CanonicalizeFunctionNumber("<wbr>canon-nth-function", cl::Hidden, cl::init(~0u),<br>
> + cl::value_desc("N"),<br>
> + cl::desc("Function number to canonicalize."));<br>
> +<br>
> +static cl::opt<unsigned><br>
> +CanonicalizeBasicBlockNumber(<wbr>"canon-nth-basicblock", cl::Hidden, cl::init(~0u),<br>
> + cl::value_desc("N"),<br>
> + cl::desc("BasicBlock number to canonicalize."));<br>
> +<br>
> +namespace {<br>
> +<br>
> +class MIRCanonicalizer : public MachineFunctionPass {<br>
> +public:<br>
> + static char ID;<br>
> + MIRCanonicalizer() : MachineFunctionPass(ID) {}<br>
> +<br>
> + StringRef getPassName() const override {<br>
> + return "Rename register operands in a canonical ordering.";<br>
> + }<br>
> +<br>
> + void getAnalysisUsage(AnalysisUsage &AU) const override {<br>
> + AU.setPreservesCFG();<br>
> + MachineFunctionPass::<wbr>getAnalysisUsage(AU);<br>
> + }<br>
> +<br>
> + bool runOnMachineFunction(<wbr>MachineFunction &MF) override;<br>
> +};<br>
> +<br>
> +} // end anonymous namespace<br>
> +<br>
</div></div>> +enum VRType { RSE_Reg = 0, RSE_FrameIndex, RSE_NewCandidate };<br>
> +class TypedVReg {<br>
<br>
I'd actually meant something as simple as:<br>
<br>
struct TypedVReg { VRType Type; unsigned Reg; }<br>
<br>
but this class with helpers to make it harder to misuse makes sense.<br>
<br>
> + VRType type;<br>
> + unsigned reg;<br>
> +<br>
> + TypedVReg(): type(RSE_Reg), reg(~0U) { }<br>
<br>
Why bother with a private default constructor? It'd be better to just<br>
not have one at all, I think.<br>
<br>
> +<br>
> +public:<br>
> + TypedVReg(unsigned _reg): type(RSE_Reg), reg(_reg) { }<br>
<br>
LLVM style usually just aliases the parameters with the member variables<br>
rather than doing the underscore thing, so just:<br>
<br>
TypedVReg(unsigned Reg): type(RSE_Reg), Reg(Reg) {}<br>
<br>
> + TypedVReg(VRType _type): type(_type), reg(~0U) { }<br>
<br>
This should probably assert that Type != RSE_Reg<br>
<br>
> + TypedVReg(const TypedVReg &TR): type(TR.type), reg(TR.reg) { }<br>
> +<br>
> + TypedVReg& operator=(const TypedVReg &rhs) {<br>
> + type = rhs.type;<br>
> + reg = rhs.reg;<br>
> + return *this;<br>
> + }<br>
<br>
These should both be default, so just omit them (or write them as =<br>
default if you're feeling verbose).<br>
<br></blockquote><div><br></div><div>I can be sure the default = operator is going to copy the members from rhs to this?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + VRType getType() const { return type; }<br>
> + unsigned getReg() const {<br>
> + assert(this->isReg() && "Expected a virtual or physical register.");<br>
> + return reg;<br>
> + }<br>
> +<br>
> + bool isReg() const { return type == RSE_Reg; }<br>
> + bool isFrameIndex() const { return type == RSE_FrameIndex; }<br>
> + bool isCandidate() const { return type == RSE_NewCandidate; }<br>
> +};<br>
<span class="">> +<br>
> +char MIRCanonicalizer::ID;<br>
> +<br>
> +char &llvm::MIRCanonicalizerID = MIRCanonicalizer::ID;<br>
> +<br>
> +INITIALIZE_PASS_BEGIN(<wbr>MIRCanonicalizer, "mir-canonicalizer",<br>
> + "Rename Register Operands Canonically", false, false);<br>
> +<br>
> +INITIALIZE_PASS_END(<wbr>MIRCanonicalizer, "mir-canonicalizer",<br>
> + "Rename Register Operands Canonically", false, false);<br>
> +<br>
> +static std::vector<MachineBasicBlock *> GetRPOList(MachineFunction &MF) {<br>
</span>> + ReversePostOrderTraversal<<wbr>MachineBasicBlock*> RPOT(&*MF.begin());<br>
<span class="">> + std::vector<MachineBasicBlock*<wbr>> RPOList;<br>
</span>> + for (auto MBB : RPOT) {<br>
<span class="">> + RPOList.push_back(MBB);<br>
> + }<br>
> +<br>
> + return RPOList;<br>
> +}<br>
> +<br>
> +// Set a dummy vreg. We use this vregs register class to generate throw-away<br>
> +// vregs that are used to skip vreg numbers so that vreg numbers line up.<br>
> +static unsigned GetDummyVReg(const MachineFunction &MF) {<br>
> + for (auto &MBB : MF) {<br>
</span>> + for (auto &MI : MBB) {<br>
> + for (auto &MO : MI.operands()) {<br>
<span class="">> + if (!MO.isReg() || !TargetRegisterInfo::<wbr>isVirtualRegister(MO.getReg())<wbr>)<br>
> + continue;<br>
> + return MO.getReg();<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + return ~0U;<br>
> +}<br>
> +<br>
> +static bool rescheduleCanoncally(<wbr>MachineBasicBlock *MBB) {<br>
> +<br>
> + bool Changed = false;<br>
> +<br>
> + // Calculates the distance of MI from the begining of its parent BB.<br>
> + auto getInstrIdx = [](const MachineInstr &MI) {<br>
> + unsigned i = 0;<br>
</span>> + for (auto &CurMI : *MI.getParent()) {<br>
> + if (&CurMI == &MI)<br>
<span class="">> + return i;<br>
> + i++;<br>
> + }<br>
</span><span class="">> + return ~0U;<br>
> + };<br>
> +<br>
> + // Pre-Populate vector of instructions to reschedule so that we don't<br>
> + // clobber the iterator.<br>
</span>> + std::vector<MachineInstr *> Instructions;<br>
> + for (auto &MI : *MBB) {<br>
> + Instructions.push_back(&MI);<br>
> + }<br>
> +<br>
> + for (auto *II : Instructions) {<br>
<span class="">> + if (II->getNumOperands() == 0)<br>
> + continue;<br>
> +<br>
> + MachineOperand &MO = II->getOperand(0);<br>
> + if (!MO.isReg() || !TargetRegisterInfo::<wbr>isVirtualRegister(MO.getReg())<wbr>)<br>
> + continue;<br>
> +<br>
> + DEBUG(dbgs() << "Operand " << 0 << " of "; II->dump(); MO.dump(););<br>
> +<br>
</span>> + MachineInstr *Def = II;<br>
> + unsigned Distance = ~0U;<br>
> + MachineInstr *UseToBringDefCloserTo = nullptr;<br>
<span class="">> + MachineRegisterInfo *MRI = &MBB->getParent()->getRegInfo(<wbr>);<br>
</span>> + for (auto &UO : MRI->use_nodbg_operands(MO.<wbr>getReg())) {<br>
> + MachineInstr *UseInst = UO.getParent();<br>
> +<br>
> + const unsigned DefLoc = getInstrIdx(*Def);<br>
> + const unsigned UseLoc = getInstrIdx(*UseInst);<br>
> + const unsigned Delta = (UseLoc - DefLoc);<br>
> +<br>
> + if (UseInst->getParent() != Def->getParent())<br>
> + continue;<br>
> + if (DefLoc >= UseLoc)<br>
> + continue;<br>
> +<br>
> + if (Delta < Distance) {<br>
> + Distance = Delta;<br>
> + UseToBringDefCloserTo = UseInst;<br>
> + }<br>
> + }<br>
> +<br>
> + const auto BBE = MBB->instr_end();<br>
> + MachineBasicBlock::iterator DefI = BBE;<br>
> + MachineBasicBlock::iterator UseI = BBE;<br>
> +<br>
> + for (auto BBI = MBB->instr_begin(); BBI != BBE; ++BBI) {<br>
> +<br>
> + if (DefI != BBE && UseI != BBE)<br>
> + break;<br>
> +<br>
> + if ((&*BBI != Def) && (&*BBI != UseToBringDefCloserTo))<br>
> + continue;<br>
> +<br>
> + if (&*BBI == Def) {<br>
> + DefI = BBI;<br>
> + continue;<br>
> + }<br>
> +<br>
> + if (&*BBI == UseToBringDefCloserTo) {<br>
> + UseI = BBI;<br>
<span class="">> + continue;<br>
> + }<br>
> + }<br>
> +<br>
</span>> + if (DefI == BBE || UseI == BBE)<br>
<span class="">> + continue;<br>
> +<br>
> + DEBUG({<br>
> + dbgs() << "Splicing ";<br>
</span>> + DefI->dump();<br>
> + dbgs() << " right before: ";<br>
> + UseI->dump();<br>
<span class="">> + });<br>
> +<br>
> + Changed = true;<br>
</span>> + MBB->splice(UseI, MBB, DefI);<br>
<span class="">> + }<br>
> +<br>
> + return Changed;<br>
> +}<br>
> +<br>
</span>> +/// Here we find our candidates. What makes an interesting candidate?<br>
> +/// An candidate for a canonicalization tree root is normally any kind of<br>
> +/// instruction that causes side effects such as a store to memory or a copy to<br>
> +/// a physical register or a return instruction. We use these as an expression<br>
> +/// tree root that we walk inorder to build a canonical walk which should result<br>
> +/// in canoncal vreg renaming.<br>
<span class="">> +static std::vector<MachineInstr *> populateCandidates(<wbr>MachineBasicBlock *MBB) {<br>
</span>> + std::vector<MachineInstr *> Candidates;<br>
<span class="">> + MachineRegisterInfo &MRI = MBB->getParent()->getRegInfo()<wbr>;<br>
> +<br>
> + for (auto II = MBB->begin(), IE = MBB->end(); II != IE; ++II) {<br>
> + MachineInstr *MI = &*II;<br>
> +<br>
</span>> + bool DoesMISideEffect = false;<br>
<span class="">> +<br>
> + if (MI->getNumOperands() > 0 && MI->getOperand(0).isReg()) {<br>
</span>> + const unsigned Dst = MI->getOperand(0).getReg();<br>
> + DoesMISideEffect |= !TargetRegisterInfo::<wbr>isVirtualRegister(Dst);<br>
> +<br>
> + for (auto UI = MRI.use_begin(Dst); UI != MRI.use_end(); ++UI) {<br>
> + if (DoesMISideEffect) break;<br>
> + DoesMISideEffect |= (UI->getParent()->getParent() != MI->getParent());<br>
> + }<br>
> + }<br>
> +<br>
> + if (!MI->mayStore() && !MI->isBranch() && !DoesMISideEffect)<br>
<span class="">> + continue;<br>
> +<br>
> + DEBUG(dbgs() << "Found Candidate: "; MI->dump(););<br>
</span>> + Candidates.push_back(MI);<br>
> + }<br>
> +<br>
> + return Candidates;<br>
> +}<br>
> +<br>
> +void doCandidateWalk(std::vector<<wbr>TypedVReg> &VRegs,<br>
> + std::queue <TypedVReg> ®_queue,<br>
<span class="">> + std::vector<MachineInstr *> &visitedMIs,<br>
<br>
</span>Looks like you missed reg_queue and visitedMIs when you were renaming<br>
variables in the llvm style.<br>
<span class=""><br>
> + const MachineBasicBlock *MBB) {<br>
> +<br>
> + const MachineFunction &MF = *MBB->getParent();<br>
> + const MachineRegisterInfo &MRI = MF.getRegInfo();<br>
> +<br>
> + while (reg_queue.size()) {<br>
> +<br>
</span>> + auto TReg = reg_queue.front();<br>
> + reg_queue.pop();<br>
> +<br>
> + if (TReg.isFrameIndex()) {<br>
<span class="">> + DEBUG(dbgs() << "Popping frame index.\n";);<br>
</span>> + VRegs.push_back(TypedVReg(RSE_<wbr>FrameIndex));<br>
> + continue;<br>
> + }<br>
> +<br>
> + assert(TReg.isReg() && "Expected vreg or physreg.");<br>
> + auto Reg = TReg.getReg();<br>
<br>
It's clearer to write out "unsigned" than to use auto here.<br>
<br>
> +<br>
> + if (TargetRegisterInfo::<wbr>isVirtualRegister(Reg)) {<br>
<span class="">> + DEBUG({<br>
> + dbgs() << "Popping vreg ";<br>
</span>> + MRI.def_begin(Reg)->dump();<br>
<span class="">> + dbgs() << "\n";<br>
> + });<br>
> +<br>
</span>> + auto VisitedVRegAt =<br>
> + std::find_if(VRegs.begin(), VRegs.end(),<br>
> + [&](const TypedVReg &TR) {<br>
> + return TR.isReg() && TR.getReg() == Reg;<br>
> + });<br>
<br>
Oops, I'd said find_if, but I really meant any_of. This can be even<br>
simpler like so:<br>
<br>
bool VisitedVReg = llvm::any_of(VRegs, [&](const TypedVReg &TR) {<br>
return TR.isReg() && TR.getReg() == Reg;<br>
});<br>
<br>
> +<br>
> + if (VRegs.end() == VisitedVRegAt)<br>
> + VRegs.push_back(TypedVReg(Reg)<wbr>);<br>
<span class="">> + } else {<br>
> + DEBUG(dbgs() << "Popping physreg.\n";);<br>
</span>> + VRegs.push_back(TypedVReg(Reg)<wbr>);<br>
> + continue;<br>
> + }<br>
> +<br>
> + for (auto RI = MRI.def_begin(Reg), RE = MRI.def_end(); RI != RE; ++RI) {<br>
> + MachineInstr *Def = RI->getParent();<br>
> +<br>
> + if (Def->getParent() != MBB)<br>
> + continue;<br>
> +<br>
> + auto VisitedMIAt =<br>
> + std::find_if(visitedMIs.begin(<wbr>), visitedMIs.end(),<br>
> + [&](const MachineInstr* VMI) { return Def == VMI; });<br>
> +<br>
> + if (visitedMIs.end() != VisitedMIAt)<br>
<span class="">> + break;<br>
> +<br>
> + DEBUG({<br>
> + dbgs() << "\n========================\n"<wbr>;<br>
> + dbgs() << "Visited MI: ";<br>
</span>> + Def->dump();<br>
> + dbgs() << "BB Name: " << Def->getParent()->getName() << "\n";<br>
<span class="">> + dbgs() << "\n========================\n"<wbr>;<br>
> + });<br>
</span>> + visitedMIs.push_back(Def);<br>
> + for (unsigned I = 1, E = Def->getNumOperands(); I != E; ++I) {<br>
> +<br>
> + MachineOperand &MO = Def->getOperand(I);<br>
<span class="">> + if (MO.isFI()) {<br>
> + DEBUG(dbgs() << "Pushing frame index.\n";);<br>
</span>> + reg_queue.push(TypedVReg(RSE_<wbr>FrameIndex));<br>
<span class="">> + }<br>
> +<br>
> + if (!MO.isReg())<br>
> + continue;<br>
</span>> + reg_queue.push(TypedVReg(MO.<wbr>getReg()));<br>
<span class="">> + }<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
</span>> +/// TODO: Work to remove this in the future. One day when we have named vregs<br>
> +/// we should be able to form the canonical name based on some characteristic<br>
> +/// we see in that point of the expression tree (like if we were to name based<br>
> +/// on some sort of value numbering scheme).<br>
<span class="">> +static void SkipVRegs(unsigned &VRegGapIndex, MachineRegisterInfo &MRI,<br>
> + const TargetRegisterClass *RC) {<br>
> + const unsigned VR_GAP = (++VRegGapIndex * 1000);<br>
> +<br>
> + DEBUG({<br>
> + dbgs() << "Adjusting per-BB VR_GAP for BB" << VRegGapIndex << " to "<br>
> + << VR_GAP << "\n";<br>
> + });<br>
> +<br>
> + unsigned I = MRI.createVirtualRegister(RC);<br>
> + const unsigned E = (((I + VR_GAP) / VR_GAP) + 1) * VR_GAP;<br>
> + while (I != E) {<br>
> + I = MRI.createVirtualRegister(RC);<br>
> + }<br>
> +}<br>
> +<br>
> +static void GetVRegRenameMap(std::map<<wbr>unsigned, unsigned> &vregRenameMap,<br>
</span>> + const std::vector<TypedVReg> &VRegs,<br>
<span class="">> + const std::vector<unsigned> &renamedInOtherBB,<br>
> + MachineRegisterInfo &MRI,<br>
> + const TargetRegisterClass *RC) {<br>
> +<br>
</span>> + unsigned LastRenameReg = MRI.createVirtualRegister(RC);<br>
> + bool FirstCandidate = true;<br>
> +<br>
> + for (auto &vreg : VRegs) {<br>
> + if (vreg.isFrameIndex()) {<br>
> + // We skip one vreg for any frame index because there is a good chance<br>
> + // (especially when comparing SelectionDAG to GlobalISel generated MIR)<br>
> + // that in the other file we are just getting an incoming vreg that comes<br>
> + // from a copy from a frame index. So it's safe to skip by one.<br>
> + LastRenameReg = MRI.createVirtualRegister(RC);<br>
> + DEBUG(dbgs() << "Skipping rename for FI " << LastRenameReg << "\n";);<br>
> + continue;<br>
> + } else if (vreg.isCandidate()) {<br>
> +<br>
> + // After the first candidate, for every subsequent candidate, we skip mod<br>
> + // 10 registers so that the candidates are more likely to start at the<br>
> + // same vreg number making it more likely that the canonical walk from the<br>
> + // candidate insruction. We don't need to skip from the first candidate of<br>
> + // the BasicBlock because we already skip ahead several vregs for each BB.<br>
> + while (LastRenameReg % 10) {<br>
> + if (!FirstCandidate) break;<br>
> + LastRenameReg = MRI.createVirtualRegister(RC);<br>
> +<br>
> + DEBUG({<br>
> + dbgs() << "Skipping rename for new candidate " << LastRenameReg<br>
<span class="">> + << "\n";<br>
> + });<br>
> + }<br>
</span>> + FirstCandidate = false;<br>
> + continue;<br>
> + } else if (!TargetRegisterInfo::<wbr>isVirtualRegister(vreg.getReg(<wbr>))) {<br>
> + LastRenameReg = MRI.createVirtualRegister(RC);<br>
> + DEBUG({<br>
> + dbgs() << "Skipping rename for Phys Reg " << LastRenameReg << "\n";<br>
<span class="">> + });<br>
> + continue;<br>
> + }<br>
> +<br>
</span>> + auto Reg = vreg.getReg();<br>
> + if (llvm::find(renamedInOtherBB, Reg) != renamedInOtherBB.end()) {<br>
> + DEBUG(dbgs() << "Vreg " << Reg << " already renamed in other BB.\n";);<br>
> + continue;<br>
> + }<br>
> +<br>
> + auto Rename = MRI.createVirtualRegister(MRI.<wbr>getRegClass(Reg));<br>
> + LastRenameReg = Rename;<br>
> +<br>
> + if (vregRenameMap.find(Reg) == vregRenameMap.end()) {<br>
<span class="">> + DEBUG(dbgs() << "Mapping vreg ";);<br>
</span>> + if (MRI.reg_begin(Reg) != MRI.reg_end()) {<br>
> + DEBUG(auto foo = &*MRI.reg_begin(Reg); foo->dump(););<br>
> + } else {<br>
> + DEBUG(dbgs() << Reg;);<br>
<span class="">> + }<br>
> + DEBUG(dbgs() << " to ";);<br>
</span>> + if (MRI.reg_begin(Rename) != MRI.reg_end()) {<br>
> + DEBUG(auto foo = &*MRI.reg_begin(Rename); foo->dump(););<br>
> + } else {<br>
> + DEBUG(dbgs() << Rename;);<br>
<span class="">> + }<br>
> + DEBUG(dbgs() << "\n";);<br>
> +<br>
</span>> + vregRenameMap.insert(std::<wbr>pair<unsigned, unsigned>(Reg, Rename));<br>
<span class="">> + }<br>
> + }<br>
> +}<br>
> +<br>
> +static bool doVRegRenaming(std::vector<<wbr>unsigned> &renamedInOtherBB,<br>
> + const std::map<unsigned, unsigned> &vregRenameMap,<br>
> + MachineRegisterInfo &MRI) {<br>
> + bool Changed = false;<br>
> + for (auto I = vregRenameMap.begin(), E = vregRenameMap.end(); I != E; ++I) {<br>
> +<br>
</span>> + auto VReg = I->first;<br>
> + auto Rename = I->second;<br>
> +<br>
> + renamedInOtherBB.push_back(<wbr>Rename);<br>
> +<br>
> + std::vector<MachineOperand *> RenameMOs;<br>
> + for (auto &MO : MRI.reg_operands(VReg)) {<br>
> + RenameMOs.push_back(&MO);<br>
> + }<br>
> +<br>
> + for (auto *MO : RenameMOs) {<br>
> + Changed = true;<br>
> + MO->setReg(Rename);<br>
<span class="">> +<br>
> + if (!MO->isDef())<br>
> + MO->setIsKill(false);<br>
> + }<br>
> + }<br>
> +<br>
> + return Changed;<br>
> +}<br>
> +<br>
> +static bool doDefKillClear(<wbr>MachineBasicBlock *MBB) {<br>
> + bool Changed = false;<br>
> +<br>
</span>> + for (auto &MI : *MBB) {<br>
> + for (auto &MO : MI.operands()) {<br>
<div><div class="h5">> + if (!MO.isReg())<br>
> + continue;<br>
> + if (!MO.isDef() && MO.isKill()) {<br>
> + Changed = true;<br>
> + MO.setIsKill(false);<br>
> + }<br>
> +<br>
> + if (MO.isDef() && MO.isDead()) {<br>
> + Changed = true;<br>
> + MO.setIsDead(false);<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + return Changed;<br>
> +}<br>
> +<br>
> +static bool runOnBasicBlock(<wbr>MachineBasicBlock *MBB,<br>
> + std::vector<StringRef> &bbNames,<br>
> + std::vector<unsigned> &renamedInOtherBB,<br>
> + unsigned &basicBlockNum, unsigned &VRegGapIndex) {<br>
> +<br>
> + if (CanonicalizeBasicBlockNumber != ~0U) {<br>
> + if (CanonicalizeBasicBlockNumber != basicBlockNum++)<br>
> + return false;<br>
> + DEBUG(dbgs() << "\n Canonicalizing BasicBlock " << MBB->getName() << "\n";);<br>
> + }<br>
> +<br>
</div></div>> + if (llvm::find(bbNames, MBB->getName()) != bbNames.end()) {<br>
<span class="">> + DEBUG({<br>
> + dbgs() << "Found potentially duplicate BasicBlocks: " << MBB->getName()<br>
> + << "\n";<br>
> + });<br>
> + return false;<br>
> + }<br>
> +<br>
> + DEBUG({<br>
> + dbgs() << "\n\n NEW BASIC BLOCK: " << MBB->getName() << " \n\n";<br>
> + dbgs() << "\n\n=========================<wbr>=======================\n\n";<br>
> + });<br>
> +<br>
> + bool Changed = false;<br>
> + MachineFunction &MF = *MBB->getParent();<br>
> + MachineRegisterInfo &MRI = MF.getRegInfo();<br>
> +<br>
</span>> + const TargetRegisterClass *DummyRC = [] (const MachineFunction &MF) {<br>
> + const unsigned DummyVReg = GetDummyVReg(MF);<br>
> + assert(DummyVReg != ~0U && "Could not find a dummy VReg.");<br>
<br>
So this pass doesn't work at all if we give it input with no vregs. That<br>
doesn't seem great - maybe we'd be better off making something up in<br>
that case.<br>
<span class=""><br></span></blockquote><div><br></div><div>Maybe I should bail out of the vreg renaming portion but continue the instruction ordering?</div><div>I think I shouldn't assert but if I can't find a vreg then in the current form of the pass im not sure exactly what I could rename in the first place. Hmm..</div><div><br></div><div>I'm still working on another patch revision. Will reply with it soon.</div><div><br></div><div>PL<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + const MachineRegisterInfo &MRI = MF.getRegInfo();<br>
</span>> + return MRI.getRegClass(DummyVReg);<br>
<span class="">> + } (MF);<br>
> +<br>
> + bbNames.push_back(MBB-><wbr>getName());<br>
> + DEBUG(dbgs() << "\n\n NEW BASIC BLOCK: " << MBB->getName() << "\n\n";);<br>
> +<br>
> + DEBUG(dbgs() << "MBB Before Scheduling:\n"; MBB->dump(););<br>
> + Changed |= rescheduleCanoncally(MBB);<br>
> + DEBUG(dbgs() << "MBB After Scheduling:\n"; MBB->dump(););<br>
> +<br>
</span>> + std::vector<MachineInstr *> Candidates = populateCandidates(MBB);<br>
> + std::vector<MachineInstr *> VisitedMIs;<br>
> + std::copy(Candidates.begin(), Candidates.end(),<br>
> + std::back_inserter(VisitedMIs)<wbr>);<br>
> +<br>
> + std::vector<TypedVReg> VRegs;<br>
> + for (auto candidate : Candidates) {<br>
> + VRegs.push_back(TypedVReg(RSE_<wbr>NewCandidate));<br>
> +<br>
> + std::queue<TypedVReg> reg_queue;<br>
<span class="">> +<br>
> + // Here we walk the vreg operands of a non-root node along our walk.<br>
> + // The root nodes are the original candidates (stores normally).<br>
> + // These are normally not the root nodes (except for the case of copies to<br>
> + // physical registers).<br>
> + for (unsigned i = 1; i < candidate->getNumOperands(); i++) {<br>
> + if (candidate->mayStore() || candidate->isBranch())<br>
> + break;<br>
> +<br>
> + MachineOperand &MO = candidate->getOperand(i);<br>
> + if (!(MO.isReg() && TargetRegisterInfo::<wbr>isVirtualRegister(MO.getReg())<wbr>))<br>
> + continue;<br>
> +<br>
> + DEBUG(dbgs() << "Enqueue register"; MO.dump(); dbgs() << "\n";);<br>
</span>> + reg_queue.push(TypedVReg(MO.<wbr>getReg()));<br>
<span class="">> + }<br>
> +<br>
> + // Here we walk the root candidates. We start from the 0th operand because<br>
> + // the root is normally a store to a vreg.<br>
> + for (unsigned i = 0; i < candidate->getNumOperands(); i++) {<br>
> +<br>
> + if (!candidate->mayStore() && !candidate->isBranch())<br>
> + break;<br>
> +<br>
> + MachineOperand &MO = candidate->getOperand(i);<br>
> +<br>
> + // TODO: Do we want to only add vregs here?<br>
> + if (!MO.isReg() && !MO.isFI())<br>
> + continue;<br>
> +<br>
> + DEBUG(dbgs() << "Enqueue Reg/FI"; MO.dump(); dbgs() << "\n";);<br>
> +<br>
</span>> + reg_queue.push(MO.isReg() ? TypedVReg(MO.getReg()) :<br>
> + TypedVReg(RSE_FrameIndex));<br>
<span class="">> + }<br>
> +<br>
> + // Now that we have pushed the operands of the candidate here, we do the<br>
> + // full breadth first walk.<br>
</span>> + doCandidateWalk(VRegs, reg_queue, VisitedMIs, MBB);<br>
<span class="">> + }<br>
> +<br>
> + // If we have populated no vregs to rename then bail.<br>
> + // The rest of this function does the vreg remaping.<br>
> + if (VRegs.size() == 0)<br>
> + return Changed;<br>
> +<br>
> + // Skip some vregs, so we can recon where we'll land next.<br>
</span>> + SkipVRegs(VRegGapIndex, MRI, DummyRC);<br>
> +<br>
> + std::map<unsigned, unsigned> VRegRenameMap;<br>
> + GetVRegRenameMap(<wbr>VRegRenameMap, VRegs, renamedInOtherBB, MRI, DummyRC);<br>
> +<br>
> + Changed |= doVRegRenaming(<wbr>renamedInOtherBB, VRegRenameMap, MRI);<br>
<div><div class="h5">> + Changed |= doDefKillClear(MBB);<br>
> +<br>
> + DEBUG(dbgs() << "Updated MachineBasicBlock:\n"; MBB->dump(); dbgs() << "\n";);<br>
> + DEBUG(dbgs() << "\n\n=========================<wbr>=======================\n\n");<br>
> + return Changed;<br>
> +}<br>
> +<br>
> +bool MIRCanonicalizer::<wbr>runOnMachineFunction(<wbr>MachineFunction &MF) {<br>
> +<br>
> + static unsigned functionNum = 0;<br>
> + if (CanonicalizeFunctionNumber != ~0U) {<br>
> + if (CanonicalizeFunctionNumber != functionNum++)<br>
> + return false;<br>
> + DEBUG(dbgs() << "\n Canonicalizing Function " << MF.getName() << "\n";);<br>
> + }<br>
> +<br>
> + // we need a valid vreg to create a vreg type for skipping all those<br>
> + // stray vreg numbers so reach alignment/canonical vreg values.<br>
> + std::vector<MachineBasicBlock*<wbr>> RPOList = GetRPOList(MF);<br>
> +<br>
> + DEBUG(<br>
> + dbgs() << "\n\n NEW MACHINE FUNCTION: " << MF.getName() << " \n\n";<br>
> + dbgs() << "\n\n=========================<wbr>=======================\n\n";<br>
> + dbgs() << "Total Basic Blocks: " << RPOList.size() << "\n";<br>
> + for (auto MBB : RPOList) {<br>
> + dbgs() << MBB->getName() << "\n";<br>
> + }<br>
> + dbgs() << "\n\n=========================<wbr>=======================\n\n";<br>
> + );<br>
> +<br>
</div></div>> + std::vector<StringRef> BBNames;<br>
> + std::vector<unsigned> RenamedInOtherBB;<br>
> +<br>
> + unsigned GapIdx = 0;<br>
> + unsigned BBNum = 0;<br>
<span class="">> +<br>
> + bool Changed = false;<br>
> +<br>
> + for (auto MBB : RPOList)<br>
</span>> + Changed |= runOnBasicBlock(MBB, BBNames, RenamedInOtherBB, BBNum, GapIdx);<br>
<div class="HOEnZb"><div class="h5">> +<br>
> + return Changed;<br>
> +}<br>
> +<br>
</div></div></blockquote></div><br></div></div>