[PATCH] D70210: [MirNamer][Canonicalizer]: Perform instruction semantic based renaming

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added a comment.

Initial comments posted. Nice work, especially on the operand/vreg-def-opcode hashing idea (which localizes lot of the difference better that before). More code comments and perhaps some ports of downstream test cases would be greatly appreciated. I think we should work together to cleanup or remove some of this stuff through a couple NFC commits so that the diff is a little easier to follow and mostly consists of what was added (with the stuff removed being removed in a separate commit).



================
Comment at: llvm/lib/CodeGen/MIRCanonicalizerPass.cpp:441
-  }
-
   Changed |= doDefKillClear(MBB);
----------------
Is there anything specifically that is needed from MIRCanonicalizerPass Given this deletion here? Can you use MIRNamerPass as your base instead and do this deletion in a later NFC commit if necessary? 


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:45
 
-      MachineOperand &MO = candidate->getOperand(i);
-      if (!(MO.isReg() && Register::isVirtualRegister(MO.getReg())))
-        continue;
+  std::map<std::string, unsigned> VRegNameCollisionMap;
 
----------------
Might want to consider a StringMap


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:71
 
-    // Here we walk the root candidates. We start from the 0th operand because
-    // the root is normally a store to a vreg.
-    for (unsigned i = 0; i < candidate->getNumOperands(); i++) {
-
-      if (!candidate->mayStore() && !candidate->isBranch())
-        break;
-
-      MachineOperand &MO = candidate->getOperand(i);
-
-      // TODO: Do we want to only add vregs here?
-      if (!MO.isReg() && !MO.isFI())
-        continue;
-
-      LLVM_DEBUG(dbgs() << "Enqueue Reg/FI"; MO.dump(); dbgs() << "\n";);
-
-      RegQueue.push(MO.isReg() ? TypedVReg(MO.getReg())
-                               : TypedVReg(RSE_FrameIndex));
+std::string NamedVRegCursor::getInstructionOpcodeHash(MachineInstr &MI) {
+  std::string S;
----------------
Comments please. 


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:81
-
-void doCandidateWalk(std::vector<TypedVReg> &VRegs,
-                     std::queue<TypedVReg> &RegQueue,
----------------
I think the candidate walk code can be left alone here, and removed in an NFC if really necessary.


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:88
+    // okay.
+    return 0; // TODO.
+  };
----------------
llvm_unreachable please. 


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:105
+bool NamedVRegCursor::renameInstsInMBB(MachineBasicBlock *MBB) {
+  std::vector<TypedVReg> VRegs;
+  std::string Prefix = "bb" + std::to_string(getCurrentBBNumber()) + "_";
----------------
I like this linear approach but I might like to keep the tree based approach as well as a toggle until we can add more tests. In the tree based approach I was trying to do the canonicalization based on the chain of operations that flow into a side effect, where here the side effects are renaming barriers? On second thought, I really like the hashing approach on the VReg-Def opcode and if you are confident it wont result in too may cases where a difference that should have remained is lost, I'd be fine with replacing all of this walking business. 

Comments (and perhaps some brief MIR snippets) on how this renaming mechanism works would be really nice to have as well.

>From what I understand, you have many test cases downstream. Can these be ported to aarch64 to bolster the testing upstream? Even the tests with 7 and 8 instructions can be useful, and I'd assume shouldn't be too difficult to port to a supported downstream target? Does this sound reasonable to you @aditya_nandakumar @bogner ??


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:35
 class NamedVRegCursor {
+  // TypedVReg and VRType are used to tell the renamer what to do at points in a
+  // sequence of values to be renamed. A TypedVReg can either contain
----------------
Did this need to me moved from MIRVRegNamerUtils.cpp to MIRVRegNamerUtils.h? Can he be done in a NFC commit? On top of that, this appears to be a copy paste from https://reviews.llvm.org/D70029. Would you like to work with me on massaging these changes along D70029 into place here? 


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:68
 
-  /// virtualVRegNumber - Book keeping of the last vreg position.
-  unsigned virtualVRegNumber;
+  unsigned CurrentBBNumber = 0;
 
----------------
Another copy paste from D70029. CurrentBBNumber is never incremented or really used for anything of import here. Why is it included in this diff? 


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:92
 
-  /// renameVRegs - For a given MachineBasicBlock, scan for side-effecting
-  /// instructions, walk the def-use from each side-effecting root (in sorted
-  /// root order) and rename the encountered vregs in the def-use graph in a
-  /// canonical ordering. This method maintains book keeping for which vregs
-  /// were already renamed in RenamedInOtherBB.
-  // @return changed
-  bool renameVRegs(MachineBasicBlock *MBB);
+  unsigned createVirtualRegisterWithName(unsigned VReg,
+                                         const std::string &Name);
----------------
Comment please. It wait for D70029 to solidify and land.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70210/new/

https://reviews.llvm.org/D70210