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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 4 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:
> 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? 


================
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());
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71182