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

[David Blaikie]

================
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.
----------------
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.

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

================
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;
----------------
This function could return Optional<unsigned> rather than bool + unsigned out parameter.

================
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())
----------------
Is this a new feature of your algorithm, or did the old algorithm do this too?

================
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
----------------
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)

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:64
@@ +63,3 @@
+  // their history.
+  void clobberHistoryForRegDescribedVars(unsigned RegNo,
+                                         DbgValueHistoryMap &HistMap,
----------------
clobberVarsUsingRegister? clobberRegisterUses?

"RegDescribed" is a bit sub-optimal in general, I think, perhaps.

Maybe "clobberHistoryForInRegVars"?

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:85
@@ +84,3 @@
+  // their history.
+  void clobberHistoryForAllRegDescribedVars(DbgValueHistoryMap &HistMap,
+                                         const MachineInstr &ClobberingInstr) {
----------------
I take it "RegDescribed" means "variables in registers" or just "variables not on the stack"? (what about variables that are indirected through a register?)

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

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

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:113
@@ +112,3 @@
+                 DbgValueHistoryMap &Result) {
+    RegDescribedVars.clear();
+
----------------
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..)

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:118
@@ +117,3 @@
+      // register-described variables.
+      const auto &LastMBBInstr = MBB.getLastNonDebugInstr();
+      bool SeenLastMBBInstr = LastMBBInstr == MBB.end();
----------------
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 *")

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:124
@@ +123,3 @@
+
+        if (MI.isDebugValue()) {
+          assert(MI.getNumOperands() > 1 && "Invalid DBG_VALUE instruction!");
----------------
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;
  }

...

http://reviews.llvm.org/D3597