[NFC][llvm][MIRVRegNamerUtils] Making some stylistic changes to MIRVRegNamerUtils.cpp

[plotfi]

Summary

There are a couple places where I think some of the style could be a bit more modern or a little nicer looking so I made them.

[compnerd]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

15–16

Why not const? The map itself isn’t modified.

[plotfi]

Updated based on @compnerd's feedback.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

15–16

ditto

[aditya_nandakumar]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

18

I honestly don't think this is actually making this code easier to read (or modernizing) compared to the push_back (there's back_inserter, transform with lambda) compared to a simple one liner for loop. Maybe you want std::copy with the backinserter - but even that I'm conflicted on readability compared to a simple for loop here.

73

Again I find the for loop easy to read and the std::transform use case not so much.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

18

I could std::copy references, but the transform here is to be more inline with the semantics of the previous for loop.

[aditya_nandakumar]

I don't know what the convention of LLVM is here - but I prefer the simple for loop with a push back vs a use of transform (which is not really transforming anything) which is confusing.

[plotfi]

In D71182#1775771, @aditya_nandakumar wrote:

I don't know what the convention of LLVM is here - but I prefer the simple for loop with a push back vs a use of transform (which is not really transforming anything) which is confusing.

In one case, the transform is very subtle, and I can revert it back to the simple for loop. But in the cast with the hashing it is absolutely doing a transform and I would like to make the change.

[aditya_nandakumar]

I don't see how it's different for the hashing case.
On one side, you have

for (auto &Op: MI.uses())
  Ops.push_back(HashOperand(Op));

On the other side,

llvm::transform(MI.uses(), std::back_inserter(Ops),
                           [&HashOperand](const MachineOperand &MO) { return HashOperand(MO); });

I definitely don't think second case is easier to read, shorter than the first case and I really see no value in using transform here when essentially all it's doing is to just add a bunch of operands to a list.

[compnerd]

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

18

I actually think this code here should be replaced with MachineRegisterInfo::replaceRegWith, and I am also not even sure if dropping the kill semantically makes sense. Was this taken from the old renamer or was this deliberately added for some specific shader here?

73

I'm sorry, but I just do not agree on this one. Here we specifically want to get a collection of hashes from a collection of uses. I think the transform is a lot more clear here.

[plotfi]

Update to make llvm::transform more clear than standard for-loop

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

18

Replaced this code with a call to MachineRegisterInfo::replaceRegWith. The if(!isDef)setIfKill(false) code I added in the original MIRCanon pass, and I am just not a fan of it here and I do not think it makes semantic sense. I think it could and should be dropped in a subsequent commit.

73

I think with this change, the new version is much more straight-forward. Are there any other concerns here? This is a NFC change. I think this should be changed here to be more inline with some of the more modern c++ and some of the STLExtras we are encouraged to use in LLVM.

[plotfi]

In D71182#1775875, @compnerd wrote:

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

[bogner]

In D71182#1776145, @plotfi wrote:

In D71182#1775875, @compnerd wrote:

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

FWIW I'm not really convinced that std::transform is *ever* clearer or better any more since C++11 gave us auto and range-for.

In any case, the rest of this all seems like a nice clean up to me.

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

110–111

Slightly simpler to use a StringRef param and then this is just std::string Temp(Name.lower())

[plotfi]

In D71182#1776159, @bogner wrote:

In D71182#1776145, @plotfi wrote:

In D71182#1775875, @compnerd wrote:

The std::transform better expresses the intent IMO. I think that it is definitely more idiomatic C++, so I think that is an improvement.

I agree with this. Was going through the changes in this util in my downtime and just saw some opportunities to spruce some things up. I find it really odd that you'd have a HashOperand lambda and not want to apply that to your collection of uses from a llvm::transform.

FWIW I'm not really convinced that std::transform is *ever* clearer or better any more since C++11 gave us auto and range-for.

In any case, the rest of this all seems like a nice clean up to me.

Thanks for this. I understand what you mean. In any case, I dropped the other weird transform and am now reusing the method in MRI for renaming the vregs. I really want to drop that weird isDef/kill modification code that was added long ago, because I am not really sure the semantic implications of it and things were kind of an experiment when those 3 lines of code were put in long ago. Will land once I do the recommended StringRef change. @aditya_nandakumar @compnerd @bogner thank you for the review guys!

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

111–116

@aditya_nandakumar do you remember the reason that in the RC case we are using the lower case name but in the typed case we are creating a generic vreg but not lower casing the name? I don't remember this. Might put a comment in here for that.

[aditya_nandakumar]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

111–116

I was only dealing with non generic vregs (post selection) - I didn't plan on using this mid GIsel pipeline - please update that as well while you can.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

111–116

Sounds good. In that case I will designate this as "NFCi"

llvm/lib/CodeGen/MIRVRegNamerUtils.h

 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/Support/raw_ostream.h"
 
-
 namespace llvm {
 /// VRegRenamer - This class is used for renaming vregs in a machine basic
 /// block according to semantics of the instruction.
 class VRegRenamer {
   class NamedVReg {
     Register Reg;
     std::string Name;
 
public:
   VRegRenamer(MachineRegisterInfo &MRI) : MRI(MRI) {}
 
   /// createVirtualRegister - Given an existing vreg, create a named vreg to
   /// take its place. The name is determined by calling
   /// getInstructionOpcodeHash.
   unsigned createVirtualRegister(unsigned VReg);
 
   /// Create a vreg with name and return it.
-  unsigned createVirtualRegisterWithName(unsigned VReg,
-                                         const std::string &Name);
+  unsigned createVirtualRegisterWithLowerName(unsigned VReg, StringRef Name);
   /// Linearly traverse the MachineBasicBlock and rename each instruction's
   /// vreg definition based on the semantics of the instruction.
   /// Names are as follows bb<BBNum>_hash_[0-9]+
   bool renameInstsInMBB(MachineBasicBlock *MBB);
 
   /// Same as the above, but sets a BBNum depending on BB traversal that
   /// will be used as prefix for the vreg names.
   bool renameVRegs(MachineBasicBlock *MBB, unsigned BBNum);
 
   unsigned getCurrentBBNumber() const { return CurrentBBNumber; }
 };
 
 } // namespace llvm
 
 #endif

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

 //===---------- MIRVRegNamerUtils.cpp - MIR VReg Renaming Utilities -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MIRVRegNamerUtils.h"
 #include "llvm/Support/Debug.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "mir-vregnamer-utils"
 
-bool VRegRenamer::doVRegRenaming(
-    const std::map<unsigned, unsigned> &VRegRenameMap) {
-  bool Changed = false;
-  for (auto I = VRegRenameMap.begin(), E = VRegRenameMap.end(); I != E; ++I) {
-
-    auto VReg = I->first;
-    auto Rename = I->second;
-
-    std::vector<MachineOperand *> RenameMOs;
-    for (auto &MO : MRI.reg_operands(VReg)) {
-      RenameMOs.push_back(&MO);
-    }
+using VRegRenameMap = std::map<unsigned, unsigned>;
 
-    for (auto *MO : RenameMOs) {
-      Changed = true;
-      MO->setReg(Rename);
+bool VRegRenamer::doVRegRenaming(const VRegRenameMap &VRM) {
+  bool Changed = false;
 
-      if (!MO->isDef())
-        MO->setIsKill(false);
-    }
+  for (const auto &E : VRM) {
+    Changed = Changed || !MRI.reg_empty(E.first);
+    MRI.replaceRegWith(E.first, E.second);
   }
 
   return Changed;
 }
 
-std::map<unsigned, unsigned>
+VRegRenameMap
 VRegRenamer::getVRegRenameMap(const std::vector<NamedVReg> &VRegs) {
-  std::map<unsigned, unsigned> VRegRenameMap;
 
-  std::map<std::string, unsigned> VRegNameCollisionMap;
+  StringMap<unsigned> VRegNameCollisionMap;
 
-  auto GetUniqueVRegName =
-      [&VRegNameCollisionMap](const NamedVReg &Reg) -> std::string {
-    auto It = VRegNameCollisionMap.find(Reg.getName());
-    unsigned Counter = 0;
-    if (It != VRegNameCollisionMap.end()) {
-      Counter = It->second;
-    }
-    ++Counter;
-    VRegNameCollisionMap[Reg.getName()] = Counter;
+  auto GetUniqueVRegName = [&VRegNameCollisionMap](const NamedVReg &Reg) {
+    if (VRegNameCollisionMap.find(Reg.getName()) == VRegNameCollisionMap.end())
+      VRegNameCollisionMap[Reg.getName()] = 0;
+    const unsigned Counter = ++VRegNameCollisionMap[Reg.getName()];
     return Reg.getName() + "__" + std::to_string(Counter);
   };
 
-  for (auto &Vreg : VRegs) {
-    auto Reg = Vreg.getReg();
-    assert(Register::isVirtualRegister(Reg) &&
-           "Expecting Virtual Registers Only");
-    auto NewNameForReg = GetUniqueVRegName(Vreg);
-    auto Rename = createVirtualRegisterWithName(Reg, NewNameForReg);
-
-    VRegRenameMap.insert(std::pair<unsigned, unsigned>(Reg, Rename));
+  VRegRenameMap VRM;
+  for (const auto &VReg : VRegs) {
+    const unsigned Reg = VReg.getReg();
+    VRM[Reg] = createVirtualRegisterWithLowerName(Reg, GetUniqueVRegName(VReg));
   }
-  return VRegRenameMap;
+  return VRM;
 }
 
 std::string VRegRenamer::getInstructionOpcodeHash(MachineInstr &MI) {
   std::string S;
   raw_string_ostream OS(S);
-  auto HashOperand = [this](const MachineOperand &MO) -> unsigned {
+
+  // Gets a hashable artifact from a given MachineOperand (ie an unsigned).
+  auto GetHashableMO = [this](const MachineOperand &MO) -> unsigned {
     if (MO.isImm())
       return MO.getImm();
     if (MO.isTargetIndex())
       return MO.getOffset() | (MO.getTargetFlags() << 16);
-    if (MO.isReg()) {
-      return Register::isVirtualRegister(MO.getReg())
-                 ? MRI.getVRegDef(MO.getReg())->getOpcode()
-                 : (unsigned)MO.getReg();
-    }
+    if (MO.isReg() && Register::isVirtualRegister(MO.getReg()))
+      return MRI.getVRegDef(MO.getReg())->getOpcode();
+    if (MO.isReg())
+      return MO.getReg();
+    // TODO:
     // We could explicitly handle all the types of the MachineOperand,
     // here but we can just return a common number until we find a
     // compelling test case where this is bad. The only side effect here
-    // is contributing to a hash collission but there's enough information
+    // is contributing to a hash collision but there's enough information
     // (Opcodes,other registers etc) that this will likely not be a problem.
     return 0;
   };
-  SmallVector<unsigned, 16> MIOperands;
-  MIOperands.push_back(MI.getOpcode());
-  for (auto &Op : MI.uses()) {
-    MIOperands.push_back(HashOperand(Op));
-  }
+
+  SmallVector<unsigned, 16> MIOperands = {MI.getOpcode()};
+  llvm::transform(MI.uses(), std::back_inserter(MIOperands), GetHashableMO);
+
   auto HashMI = hash_combine_range(MIOperands.begin(), MIOperands.end());
   return std::to_string(HashMI).substr(0, 5);
 }
 
 unsigned VRegRenamer::createVirtualRegister(unsigned VReg) {
-  return createVirtualRegisterWithName(
-      VReg, getInstructionOpcodeHash(*MRI.getVRegDef(VReg)));
+  assert(Register::isVirtualRegister(VReg) && "Expected Virtual Registers");
+  std::string Name = getInstructionOpcodeHash(*MRI.getVRegDef(VReg));
+  return createVirtualRegisterWithLowerName(VReg, Name);
 }
 
 bool VRegRenamer::renameInstsInMBB(MachineBasicBlock *MBB) {
   std::vector<NamedVReg> VRegs;
   std::string Prefix = "bb" + std::to_string(getCurrentBBNumber()) + "_";
-  for (auto &MII : *MBB) {
-    MachineInstr &Candidate = MII;
+  for (MachineInstr &Candidate : *MBB) {
     // Don't rename stores/branches.
     if (Candidate.mayStore() || Candidate.isBranch())
       continue;
     if (!Candidate.getNumOperands())
       continue;
     // Look for instructions that define VRegs in operand 0.
     MachineOperand &MO = Candidate.getOperand(0);
     // Avoid non regs, instructions defining physical regs.
     if (!MO.isReg() || !Register::isVirtualRegister(MO.getReg()))
       continue;
     VRegs.push_back(
         NamedVReg(MO.getReg(), Prefix + getInstructionOpcodeHash(Candidate)));
   }
 
-  // If we have populated no vregs to rename then bail.
-  // The rest of this function does the vreg remaping.
-  if (VRegs.size() == 0)
-    return false;
-
-  auto VRegRenameMap = getVRegRenameMap(VRegs);
-  return doVRegRenaming(VRegRenameMap);
+  return VRegs.size() ? doVRegRenaming(getVRegRenameMap(VRegs)) : false;
 }
 
 bool VRegRenamer::renameVRegs(MachineBasicBlock *MBB, unsigned BBNum) {
   CurrentBBNumber = BBNum;
   return renameInstsInMBB(MBB);
 }
 
-unsigned VRegRenamer::createVirtualRegisterWithName(unsigned VReg,
-                                                    const std::string &Name) {
-  std::string Temp(Name);
-  std::transform(Temp.begin(), Temp.end(), Temp.begin(), ::tolower);
-  if (auto RC = MRI.getRegClassOrNull(VReg))
-    return MRI.createVirtualRegister(RC, Temp);
-  return MRI.createGenericVirtualRegister(MRI.getType(VReg), Name);
+unsigned VRegRenamer::createVirtualRegisterWithLowerName(unsigned VReg,
+                                                         StringRef Name) {
+  std::string LowerName = Name.lower();
+  const TargetRegisterClass *RC = MRI.getRegClassOrNull(VReg);
+  return RC ? MRI.createVirtualRegister(RC, LowerName)
+            : MRI.createGenericVirtualRegister(MRI.getType(VReg), LowerName);
 }