[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

21–28

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

[plotfi]

Updated based on @compnerd's feedback.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

21–28

ditto

[aditya_nandakumar]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

37

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.

89

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

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

37

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

37

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?

89

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

37

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.

89

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

129

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

132

@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

132

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

132

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

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) {
+using VRegRenameMap = std::map<unsigned, unsigned>;
+
+bool VRegRenamer::doVRegRenaming(const VRegRenameMap &VRM) {
   bool Changed = false;
-  for (auto I = VRegRenameMap.begin(), E = VRegRenameMap.end(); I != E; ++I) {
 
-    auto VReg = I->first;
-    auto Rename = I->second;
+  for (const auto &E : VRM) {
+    unsigned VReg = E.first;
+    unsigned Rename = E.second;
+    assert(Register::isVirtualRegister(VReg) && "Expected Virtual Registers");
+    if (MRI.reg_empty(VReg))
+      continue;
+    Changed = true;
+    MRI.replaceRegWith(VReg, Rename);
 
+    // TODO: Not sure if this makes sense anymore. Do we really want to drop
+    // semantic information like def/kill? Remove the code below in a followup
+    // commit.
     std::vector<MachineOperand *> RenameMOs;
-    for (auto &MO : MRI.reg_operands(VReg)) {
+    for (auto &MO : MRI.reg_operands(VReg))
       RenameMOs.push_back(&MO);
-    }
-
-    for (auto *MO : RenameMOs) {
-      Changed = true;
-      MO->setReg(Rename);
-
+    for (auto *MO : RenameMOs)
       if (!MO->isDef())
         MO->setIsKill(false);
-    }
-  }
 
   return Changed;
 }
 
 std::map<unsigned, unsigned>
 VRegRenamer::getVRegRenameMap(const std::vector<NamedVReg> &VRegs) {
-  std::map<unsigned, unsigned> VRegRenameMap;
 
   std::map<std::string, 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;
-    }
+    unsigned Counter = (It != VRegNameCollisionMap.end()) ? It->second : 0;
     ++Counter;
     VRegNameCollisionMap[Reg.getName()] = Counter;
     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) {
+    unsigned Reg = Vreg.getReg();
+    assert(Register::isVirtualRegister(Reg) && "Expected Virtual Registers");
+    VRM[Reg] = createVirtualRegisterWithName(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 {
     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();
     }
     // 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
     // (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), HashOperand);
+
   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)));
 }
 
 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);
+  llvm::transform(Temp, Temp.begin(), ::tolower);
   if (auto RC = MRI.getRegClassOrNull(VReg))
     return MRI.createVirtualRegister(RC, Temp);
   return MRI.createGenericVirtualRegister(MRI.getType(VReg), Name);
 }