[PATCH] D67114: [mir-canon][NFC] Move MIR Vreg renaming code to separate file for better reuse.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

This is just code movement, but still a couple of nits/questions.

Only thing I think that I'd really like to see here is some documentation in the header file for functions, classes, etc. If this is going to be shared between some passes, I think it makes sense to document what each thing does, and how these things fit together.

And, since we're moving it all anyway, I guess it'd be worth making function/variable names fit the standard style while we're here.



================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:15-20
+/// Here we find our candidates. What makes an interesting candidate?
+/// An candidate for a canonicalization tree root is normally any kind of
+/// instruction that causes side effects such as a store to memory or a copy to
+/// a physical register or a return instruction. We use these as an expression
+/// tree root that we walk inorder to build a canonical walk which should result
+/// in canoncal vreg renaming.
----------------
- Move Doxygen comment to the header?
- s/An/A/



================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:21
+/// in canoncal vreg renaming.
+std::vector<MachineInstr *> populateCandidates(MachineBasicBlock *MBB) {
+  std::vector<MachineInstr *> Candidates;
----------------
Now that this isn't fully tied to the canonicalizer, I think it would make sense to give it a more descriptive name?

E.g. `findInstructionsWithSideEffects` or something?


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:51
+
+void doCandidateWalk(std::vector<TypedVReg> &VRegs,
+                     std::queue<TypedVReg> &RegQueue,
----------------
Same thing here? Basically, anything with "candidate" makes less sense outside the context of a specific pass.

(Maybe it would make sense to pack all of this into a `Renamer` struct/class or something. I don't have any strong opinions there though.)


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:29
+enum VRType { RSE_Reg = 0, RSE_FrameIndex, RSE_NewCandidate };
+class TypedVReg {
+  VRType type;
----------------
I guess from the name, in concept it's obvious what this class *is*, but it would be nice to have a Doxygen comment documenting what its purpose is etc.


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:30
+class TypedVReg {
+  VRType type;
+  unsigned reg;
----------------
While we're here, might as well rename things to fit the style guide?

e.g. `Type`, `Reg`, etc.


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:31
+  VRType type;
+  unsigned reg;
+
----------------
Use `Register` instead of `unsigned`? Or is this your own thing? (We started using `Register` instead of unsigned for registers recently.)


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:50
+
+class NamedVRegCursor {
+  MachineRegisterInfo &MRI;
----------------
This class could use a Doxygen comment as well. It's a little less obvious what it does.


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:57
+
+  void SkipVRegs() {
+    unsigned VRegGapIndex = 1;
----------------
`skipVRegs`


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:63
+    }
+    const unsigned VR_GAP = (++VRegGapIndex * 1000);
+
----------------
Why 1000?


================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.h:100
+std::map<unsigned, unsigned>
+GetVRegRenameMap(const std::vector<TypedVReg> &VRegs,
+                 const std::vector<unsigned> &renamedInOtherBB,
----------------
`getVRegRenameMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67114