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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 2 inline comments as done.
plotfi added inline comments.


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:28
+                    [](MachineOperand &MO) { return &MO; });
+    Changed = Changed || RenameMOs.size();
 
----------------
plotfi wrote:
> plotfi wrote:
> > aditya_nandakumar wrote:
> > > 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.
> > I could std::copy references, but the transform here is to be more inline with the semantics of the previous for loop. 
> 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? 
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.  


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:90
+      MI.uses(), std::back_inserter(MIOperands),
+      [&HashOperand](const MachineOperand &MO) { return HashOperand(MO); });
   auto HashMI = hash_combine_range(MIOperands.begin(), MIOperands.end());
----------------
plotfi wrote:
> aditya_nandakumar wrote:
> > Again I find the for loop easy to read and the std::transform use case not so much.
> 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. 
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71182