<div dir="ltr">Thanks Justin!<div><br></div><div>I have an updated patch, addressing the provided feedback.<div><br></div><div>Thanks Again</div></div><div><br></div><div>-Puyan</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 24, 2017 at 8:04 PM, 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>
> Hi<br>
><br>
> I've reorganized my mir-canon code into a single pass. Standalone tool is<br>
> now gone based on community feedback.<br>
<br>
</span>This is looking pretty good. A bunch of style and consistency nitpicks<br>
below. Thanks for working on this!<br>
<span class=""><br>
> Next steps will be to work on vreg naming issues in llvm (as described in<br>
> the mir-canon rfc, skipping lots of vregs can get expensive as far as<br>
> compile time goes).<br>
><br>
> PL<br>
</span>> commit 6e8ab7e06228fb3d67510cda130e92<wbr>62a81f148a<br>
> Author: Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>><br>
> Date:   Mon Oct 23 14:41:00 2017 -0700<br>
><br>
>     mir-canon: 1st patch (simplified).<br>
<br>
I'm assuming you'll put some detail about what what this is for in the<br>
message when you're ready to commit.<br>
<br>
> diff --git a/include/llvm/<wbr>InitializePasses.h b/include/llvm/<wbr>InitializePasses.h<br>
> index bf54b6471f4..12d2e9adc34 100644<br>
> --- a/include/llvm/<wbr>InitializePasses.h<br>
> +++ b/include/llvm/<wbr>InitializePasses.h<br>
> @@ -377,6 +377,7 @@ void initializeWinEHPreparePass(<wbr>PassRegistry&);<br>
>  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>
> index 00000000000..728995d0d11<br>
> --- /dev/null<br>
> +++ b/lib/CodeGen/<wbr>MIRCanonicalizerPass.cpp<br>
> @@ -0,0 +1,611 @@<br>
> +//===-------------- 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>
> +// Reorders instructions canonically.<br>
> +// Renames virtual register operands canonically.<br>
> +// Strips certain MIR artifacts (optionally).<br>
<br>
It would be better to write a couple of sentences here about what the<br>
pass is for, rather than these details about how it does it.<br>
<br>
> +//<br>
> +//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
> +<br>
> +#include "llvm/CodeGen/Passes.h"<br>
> +#include "llvm/CodeGen/<wbr>MachineInstrBuilder.h"<br>
> +#include "llvm/CodeGen/<wbr>MachineFunctionPass.h"<br>
> +#include "llvm/CodeGen/<wbr>MachineRegisterInfo.h"<br>
> +<br>
> +#include "llvm/Support/raw_ostream.h"<br>
> +#include "llvm/ADT/PostOrderIterator.h"<br>
> +#include "llvm/Target/TargetInstrInfo.<wbr>h"<br>
> +<br>
> +#include <tuple><br>
> +#include <queue><br>
<br>
Please sort each section of includes by name. clang-format can do that<br>
for you.<br>
<br>
> +<br>
> +using namespace llvm;<br>
> +<br>
> +#include <cassert><br>
<br>
This is probably included transitively by the other llvm headers anyway,<br>
in which case you can just remove it. If not, please put it up with the<br>
other stdlib headers.<br>
<br>
> +namespace llvm {<br>
> +/// The purpose of this pass is to establish a canonical operand naming so<br>
> +/// that 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.<br>
<br>
This is more or less the comment I was looking for up above in the<br>
header. Just moving this there would make sense :)<br>
<br>
> +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>
> +enum RSType { RSE_Reg = 0, RSE_FrameIndex, RSE_NewCandidate };<br>
> +typedef std::tuple<RSType, unsigned> typedRegStackEntry;<br>
<br>
I'd probably do a simple struct instead of a tuple here - names are<br>
nicer than std::get and with brace initialization you don't even need to<br>
write a constructor, so you aren't really saving anything with the tuple.<br>
<br>
> +<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>
> +  MachineBasicBlock *Entry = &*MF.begin();<br>
> +  std::vector<MachineBasicBlock*<wbr>> RPOList;<br>
> +  ReversePostOrderTraversal<<wbr>MachineBasicBlock*> RPOT(Entry);<br>
> +  for (ReversePostOrderTraversal<<wbr>MachineBasicBlock*>::rpo_<wbr>iterator<br>
> +       MBBI = RPOT.begin(), MBBE = RPOT.end(); MBBI != MBBE; ++MBBI)> {<br>
<br>
Why not use a range-for here?<br>
<br>
> +    MachineBasicBlock *MBB = *MBBI;<br>
> +    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>
> +    for (auto II = MBB.begin(), IE = MBB.end(); II != IE; ++II) {<br>
<br>
Same here.<br>
<br>
> +      const MachineInstr *MI = &*II;<br>
> +      for (unsigned i = 0; i < MI->getNumOperands(); i++) {<br>
> +        const MachineOperand &MO = MI->getOperand(i);<br>
<br>
And again here, by using MI->operands()<br>
<br>
> +        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>
> +    for (auto &I : *MI.getParent()) {<br>
> +      if (&I == &MI)<br>
> +        return i;<br>
> +      i++;<br>
> +    }<br>
<br>
Please don't use i and I here, it's confusing to have two variables that<br>
only differ by case. I'd probably call the instruction Cur or CurMI or<br>
something and use a capital I for the index.<br>
<br>
> +    return ~0U;<br>
> +  };<br>
> +<br>
> +  // Pre-Populate vector of instructions to reschedule so that we don't<br>
> +  // clobber the iterator.<br>
> +  std::vector<MachineInstr *> instructions;<br>
> +  for (auto II = MBB->begin(); II != MBB->end(); ++II) {<br>
<br>
More range-for<br>
<br>
> +    instructions.push_back(&*II);<br>
> +  }<br>
> +<br>
> +  for (auto II : instructions) {<br>
<br>
Saying "auto *II" makes it a bit more obvious at a glance that there's<br>
nothing interesting being copied here.<br>
<br>
> +    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>
> +    MachineInstr *def = II;<br>
> +    unsigned distance = ~0U;<br>
> +    MachineInstr *useToBringDefCloserTo = nullptr;<br>
> +    MachineRegisterInfo *MRI = &MBB->getParent()->getRegInfo(<wbr>);<br>
> +    for (auto UI = MRI->use_begin(MO.getReg()), UE = MRI->use_end();<br>
> +         UI != UE; ++UI) {<br>
<br>
There's probably some API on MRI to do a range for here too (MRI->uses()<br>
maybe?).<br>
<br>
> +      MachineInstr *useInst = UI->getParent();<br>
> +<br>
> +      const unsigned defLoc = getInstrIdx(*def);<br>
> +      const unsigned useLoc = getInstrIdx(*useInst);<br>
> +      const unsigned delta = (useLoc - defLoc);<br>
<br>
These variables should start with capital letters for consistency. There<br>
are some more variables that start with lower case letter elsewhere in<br>
the patch to clean up too.<br>
<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>
> +    MachineBasicBlock::iterator defI = MBB->instr_end();<br>
> +    MachineBasicBlock::iterator useI = MBB->instr_end();<br>
> +<br>
> +    for (auto BBI = MBB->instr_begin(); BBI != MBB->instr_end(); ++BBI) {<br>
<br>
Maybe do "BBE = MBB->instr_end()" in the init here and use it in the<br>
places where you call instr_end() again below?<br>
<br>
> +<br>
> +      if (defI != MBB->instr_end() && useI != MBB->instr_end())<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>
> +        continue;<br>
> +      }<br>
> +    }<br>
> +<br>
> +    if (defI == MBB->instr_end() || useI == MBB->instr_end())<br>
> +      continue;<br>
> +<br>
> +    DEBUG({<br>
> +      dbgs() << "Splicing ";<br>
> +      defI->dump();<br>
> +      dbgs() << " right before: ";<br>
> +      useI->dump();<br>
> +    });<br>
> +<br>
> +    Changed = true;<br>
> +    MBB->splice(useI, MBB, defI);<br>
> +  }<br>
> +<br>
> +  return Changed;<br>
> +}<br>
> +<br>
> +static std::vector<MachineInstr *> populateCandidates(<wbr>MachineBasicBlock *MBB) {<br>
> +  std::vector<MachineInstr *> candidates;<br>
> +  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>
> +    bool doesMISideEffect = false;<br>
> +<br>
> +    if (MI->getNumOperands() > 0 && MI->getOperand(0).isReg()) {<br>
> +      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>
A comment explaining what makes n interesting candidate would be helpful<br>
when reading this part.<br>
<br>
> +    }<br>
> +<br>
> +    if (!MI->mayStore() && !MI->isBranch() && !doesMISideEffect)<br>
> +      continue;<br>
> +<br>
> +    DEBUG(dbgs() << "Found Candidate:  "; MI->dump(););<br>
> +    candidates.push_back(MI);<br>
> +  }<br>
> +<br>
> +  return candidates;<br>
> +}<br>
> +<br>
> +void doCandidateWalk(std::vector<<wbr>typedRegStackEntry> &VRegs,<br>
> +                     std::queue <typedRegStackEntry> &reg_queue,<br>
> +                     std::vector<MachineInstr *> &visitedMIs,<br>
> +                     const MachineBasicBlock *MBB) {<br>
> +<br>
> +  const MachineFunction &MF = *MBB->getParent();<br>
> +  const MachineRegisterInfo &MRI = MF.getRegInfo();<br>
> +<br>
> +  while (reg_queue.size()) {<br>
> +    auto regType = std::get<0>(reg_queue.front())<wbr>;<br>
> +    auto reg = std::get<1>(reg_queue.front())<wbr>;<br>
> +    reg_queue.pop();<br>
> +<br>
> +    if (regType == RSE_FrameIndex) {<br>
> +      DEBUG(dbgs() << "Popping frame index.\n";);<br>
> +      VRegs.push_back(std::make_<wbr>tuple(RSE_FrameIndex, ~0U));<br>
> +      continue;<br>
> +    }<br>
> +<br>
> +    assert(regType == RSE_Reg && "Expected vreg or physreg.");<br>
> +<br>
> +    if (TargetRegisterInfo::<wbr>isVirtualRegister(reg)) {<br>
> +      DEBUG({<br>
> +        dbgs() << "Popping vreg ";<br>
> +        MRI.def_begin(reg)->dump();<br>
> +        dbgs() << "\n";<br>
> +      });<br>
> +<br>
> +      bool hasVisitedVReg = false;<br>
> +      for (auto vreg : VRegs) {<br>
> +        if (std::get<1>(vreg) == reg) {<br>
> +          hasVisitedVReg = true;<br>
> +          break;<br>
> +        }<br>
> +      }<br>
<br>
find/find_if is probably shorter.<br>
<br>
> +<br>
> +      if (!hasVisitedVReg)<br>
> +        VRegs.push_back(std::make_<wbr>tuple(RSE_Reg, reg));<br>
> +    } else {<br>
> +      DEBUG(dbgs() << "Popping physreg.\n";);<br>
> +      VRegs.push_back(std::make_<wbr>tuple(RSE_Reg, reg));<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>
> +      bool hasVisited = false;<br>
> +      for (auto VMI : visitedMIs) {<br>
> +        if (def == VMI) {<br>
> +          hasVisited = true;<br>
> +          break;<br>
> +        }<br>
> +      }<br>
<br>
Same here.<br>
<br>
> +<br>
> +      if (hasVisited)<br>
> +        break;<br>
> +<br>
> +      DEBUG({<br>
> +        dbgs() << "\n========================\n"<wbr>;<br>
> +        dbgs() << "Visited MI: ";<br>
> +        def->dump();<br>
> +        dbgs() << "BB Name: " << def->getParent()->getName() << "\n";<br>
> +        dbgs() << "\n========================\n"<wbr>;<br>
> +      });<br>
> +      visitedMIs.push_back(def);<br>
> +      for (unsigned I = 1, E = def->getNumOperands(); I != E; ++I) {<br>
> +<br>
> +        MachineOperand &MO = def->getOperand(I);<br>
> +        if (MO.isFI()) {<br>
> +          DEBUG(dbgs() << "Pushing frame index.\n";);<br>
> +          reg_queue.push(std::make_<wbr>tuple(RSE_FrameIndex, ~0U));<br>
> +        }<br>
> +<br>
> +        if (!MO.isReg())<br>
> +          continue;<br>
> +        reg_queue.push(std::make_<wbr>tuple(RSE_Reg, MO.getReg()));<br>
> +      }<br>
> +    }<br>
> +  }<br>
> +}<br>
> +<br>
> +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>
> +                             const std::vector<<wbr>typedRegStackEntry> &VRegs,<br>
> +                             const std::vector<unsigned> &renamedInOtherBB,<br>
> +                             MachineRegisterInfo &MRI,<br>
> +                             const TargetRegisterClass *RC) {<br>
> +<br>
> +  unsigned lastRenameReg = MRI.createVirtualRegister(RC);<br>
> +<br>
> +  bool firstCandidate = true;<br>
> +  for (auto vreg : VRegs) {<br>
<br>
You probably don't want copies here.<br>
<br>
> +<br>
> +    auto regType = std::get<0>(vreg);<br>
> +    auto reg = std::get<1>(vreg);<br>
> +<br>
> +    if (regType == RSE_FrameIndex) {<br>
> +      lastRenameReg = MRI.createVirtualRegister(RC);<br>
> +      DEBUG(dbgs() << "Skipping rename for FI " << lastRenameReg << "\n";);<br>
> +      continue;<br>
> +    } else if (regType == RSE_NewCandidate) {<br>
> +      if (!firstCandidate) {<br>
> +        while (lastRenameReg % 10) {<br>
> +          lastRenameReg = MRI.createVirtualRegister(RC);<br>
<br>
A comment like "round register number to the nearest ten to minimize<br>
register id differences" or something would be good here.<br>
<br>
> +<br>
> +          DEBUG({<br>
> +            dbgs() << "Skipping rename for new candidate " << lastRenameReg<br>
> +                   << "\n";<br>
> +          });<br>
> +        }<br>
> +      }<br>
> +      firstCandidate = false;<br>
> +      continue;<br>
> +    } else if (!TargetRegisterInfo::<wbr>isVirtualRegister(reg)) {<br>
> +      lastRenameReg = MRI.createVirtualRegister(RC);<br>
> +      DEBUG({<br>
> +        dbgs() << "Skipping rename for Phys Reg " << lastRenameReg << "\n";<br>
> +      });<br>
> +      continue;<br>
> +    }<br>
> +<br>
> +    if (std::find(renamedInOtherBB.<wbr>begin(), renamedInOtherBB.end(), reg) !=<br>
> +        renamedInOtherBB.end()) {<br>
<br>
There's an llvm::find in ADT/STLExtras.h that works on ranges to make this a<br>
little shorter.<br>
<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>
> +      DEBUG(dbgs() << "Mapping vreg ";);<br>
> +      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>
> +      }<br>
> +      DEBUG(dbgs() << " to ";);<br>
> +      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>
> +      }<br>
> +      DEBUG(dbgs() << "\n";);<br>
> +<br>
> +      vregRenameMap.insert(std::<wbr>pair<unsigned, unsigned>(reg, rename));<br>
> +    }<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>
> +    auto vreg = I->first;<br>
> +    auto rename = I->second;<br>
> +<br>
> +    renamedInOtherBB.push_back(<wbr>rename);<br>
> +<br>
> +    std::vector<MachineOperand *> renameMOs;<br>
> +    for (MachineOperand &MO : MRI.reg_operands(vreg)) {<br>
> +      renameMOs.push_back(&MO);<br>
> +    }<br>
> +<br>
> +    for (auto MO : renameMOs) {<br>
<br>
Another good place for "auto *"<br>
<br>
> +      Changed = true;<br>
> +      MO->setReg(rename);<br>
> +<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>
> +  for (auto II = MBB->begin(), IE = MBB->end(); II != IE; ++II) {<br>
> +    MachineInstr *MI = &*II;<br>
> +<br>
> +    for (unsigned i = 0; i < MI->getNumOperands(); i++) {<br>
> +      MachineOperand &MO = MI->getOperand(i);<br>
> +      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>
> +  if (std::find(bbNames.begin(), bbNames.end(), MBB->getName()) !=<br>
> +      bbNames.end()) {<br>
> +    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>
> +  const TargetRegisterClass *dummyRC = [] (const MachineFunction &MF) {<br>
> +    const unsigned dummyVReg = GetDummyVReg(MF);<br>
> +    assert(dummyVReg != ~0U && "Could not find a dummy VReg.");<br>
> +    const MachineRegisterInfo &MRI = MF.getRegInfo();<br>
> +    return MRI.getRegClass(dummyVReg);<br>
> +  } (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>
> +  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<<wbr>typedRegStackEntry> VRegs;<br>
> +  for (auto candidate : candidates) {<br>
> +    VRegs.push_back(std::make_<wbr>tuple(RSE_NewCandidate, ~0U));<br>
> +<br>
> +    std::queue<typedRegStackEntry> reg_queue;<br>
> +<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>
> +      reg_queue.push(std::make_<wbr>tuple(RSE_Reg, MO.getReg()));<br>
> +    }<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>
> +      reg_queue.push(MO.isReg() ? std::make_tuple(RSE_Reg, MO.getReg()) :<br>
> +                                  std::make_tuple(RSE_<wbr>FrameIndex,  ~0U));<br>
> +    }<br>
> +<br>
> +    // Now that we have pushed the operands of the candidate here, we do the<br>
> +    // full breadth first walk.<br>
> +    doCandidateWalk(VRegs, reg_queue, visitedMIs, MBB);<br>
> +  }<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>
> +  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>
> +  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>
> +  std::vector<StringRef> bbNames;<br>
> +  std::vector<unsigned> renamedInOtherBB;<br>
> +<br>
> +  unsigned gapIdx = 0;<br>
> +  unsigned bbNum = 0;<br>
> +<br>
> +  bool Changed = false;<br>
> +<br>
> +  for (auto MBB : RPOList)<br>
> +    Changed |= runOnBasicBlock(MBB, bbNames, renamedInOtherBB, bbNum, gapIdx);<br>
> +<br>
> +  return Changed;<br>
> +}<br>
> +<br>
</blockquote></div><br></div>