[PATCH] Rewrite calculateDbgValueHistory to make it (hopefully) more transparent.

[Alexey Samsonov]

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:25
@@ -29,6 +24,3 @@
 
-void calculateDbgValueHistory(const MachineFunction *MF,
-                              const TargetRegisterInfo *TRI,
-                              DbgValueHistoryMap &Result) {
-  // LiveUserVar - Map physreg numbers to the MDNode they contain.
-  std::vector<const MDNode *> LiveUserVar(TRI->getNumRegs());
+class DbgValueHistoryCalculator {
+  // RegDescribedVars - Map physreg numbers to the variables they describe.
----------------
David Blaikie wrote:
> Not sure about making a local class here - I'd probably just pass the map around to a few functions for now - but I haven't read this all yet, perhaps it makes more sense the way you've got it here.
Got rid of the class as suggested.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:33
@@ +32,3 @@
+  bool isDescribedByReg(const MachineInstr &MI, unsigned &RegNo) {
+    if (MI.isDebugValue() && MI.getNumOperands() == 3 &&
+        MI.getOperand(0).isReg() && MI.getOperand(0).getReg() &&
----------------
David Blaikie wrote:
> This long condition could probably be inverted to a series of early returns with comments describing what each case represents.
> 
> As it stands it's a pretty opaque query of various operands and their types.
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:37
@@ +36,3 @@
+         (MI.getOperand(1).isReg() && MI.getOperand(1).getReg() == 0U))) {
+      RegNo = MI.getOperand(0).getReg();
+      return true;
----------------
David Blaikie wrote:
> This function could return Optional<unsigned> rather than bool + unsigned out parameter.
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:51
@@ +50,3 @@
+    VarSet.erase(VarPos);
+    // Don't keep empty sets in a map to keep it as small as possible.
+    if (VarSet.empty())
----------------
David Blaikie wrote:
> Is this a new feature of your algorithm, or did the old algorithm do this too?
This data structure is different in my implementation. Current code just uses std::vector of size TRI->getNumRegs() instead of map. Yes, it assumes that there's at most one variable for each register.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:61
@@ +60,3 @@
+
+  // clobberHistoryForAllRegDescribedVars - Terminate the location range for
+  // variables described by register @RegNo by inserting @ClobberingInstr to
----------------
David Blaikie wrote:
> The current LLVM convention is to not repeat the function name in the documentation comment (also doxy comments should be /// not //). either omit it entirely, or if use \brief if the comment is brief (I forget when/why \brief is used/not used, though)
Done (added \brief to comments).

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:64
@@ +63,3 @@
+  // their history.
+  void clobberHistoryForRegDescribedVars(unsigned RegNo,
+                                         DbgValueHistoryMap &HistMap,
----------------
David Blaikie wrote:
> clobberVarsUsingRegister? clobberRegisterUses?
> 
> "RegDescribed" is a bit sub-optimal in general, I think, perhaps.
> 
> Maybe "clobberHistoryForInRegVars"?
Renamed to clobberRegisterUses/clobberAllRegisterUses.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:85
@@ +84,3 @@
+  // their history.
+  void clobberHistoryForAllRegDescribedVars(DbgValueHistoryMap &HistMap,
+                                         const MachineInstr &ClobberingInstr) {
----------------
David Blaikie wrote:
> I take it "RegDescribed" means "variables in registers" or just "variables not on the stack"? (what about variables that are indirected through a register?)
It means variables in a register (DBG_VALUE %reg %reg), or indirected through register (DBG_VALUE %reg %imm). I can't think of a better name for it.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:99
@@ +98,3 @@
+        continue;
+      for (MCRegAliasIterator AI(MO.getReg(), TRI, true); AI.isValid();
+           ++AI) {
----------------
David Blaikie wrote:
> Sidenote: I wonder if MCRegAliasIterator could be a real iterator one day & this could use range-based for.
Ack.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:109
@@ +108,3 @@
+public:
+  DbgValueHistoryCalculator() {}
+
----------------
David Blaikie wrote:
> unnecessary default ctor - you can just omit this
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:113
@@ +112,3 @@
+                 DbgValueHistoryMap &Result) {
+    RegDescribedVars.clear();
+
----------------
David Blaikie wrote:
> Yeah, sort of feel like the RegDescribedVars could be passed through and this function (calculate) could be inlined into calculateDbgValueHistory. Then it wouldn't be weird about whether this DbgValueHistoryCalculator can be reused or not (it's not reused in its one use case, but it clears RegDescribedVars so it could be reused..)
Done.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:118
@@ +117,3 @@
+      // register-described variables.
+      const auto &LastMBBInstr = MBB.getLastNonDebugInstr();
+      bool SeenLastMBBInstr = LastMBBInstr == MBB.end();
----------------
David Blaikie wrote:
> A const reference to an iterator is a bit weird. I'd probably use it by value (or is an iterator also an instruction, in which case I'd probably write it as "auto *" or "Instructior *")
Dropped the reference.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:124
@@ +123,3 @@
+
+        if (MI.isDebugValue()) {
+          assert(MI.getNumOperands() > 1 && "Invalid DBG_VALUE instruction!");
----------------
David Blaikie wrote:
> This 'if' is pretty long & the various things it's doing aren't exactly obvious - any way to split them out into some more named functions that would make it easier to read the high level algorithm? Maybe it's just some higher level comments, I'm not sure.
> 
> I'm not sure it would be better, but this could be inverted for less nesting:
> 
>   if (!MI.isDebugValue()) {
>     handleClobberedRegs(MI, TRI, Result);
>     continue;
>   }
> 
> ...
>   
Used last suggestion to reduce the nesting. Moved recalculating contents of RegDescribedVars to a separate function.

http://reviews.llvm.org/D3597