[PATCH] D152536: [DWARF] Allow op-index in line number programs

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Largely LGTM, some comments inline.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:643-644
+  uint64_t AddrOffset =
+      ((Row.OpIndex + OperationAdvance) * LineTable->Prologue.MinInstLength) /
+      MaxOpsPerInst;
   Row.Address.Address += AddrOffset;
----------------
If I understand correctly this calculation is incorrect, the division by MaxOpsPerInst comes before the multiply by MinInstLength.

For example, if `MaxOpsPerInst = 4, MinInstLength = 8, Row.OpIndex = 1, OperationAdvance = 2`, then this calculation would give `AddrOffset = ((2 + 1) * 8) / 4 = 6`, when it //should// be `((2 + 1) / 4) * 8 = 0` since we're actually advancing to a different OpIndex within the same instruction.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1253-1254
+  //
+  // TODO: This function, and its users, needs to be update to return multiple
+  // rows for bundles with multiple op-indexes.
   DWARFDebugLine::Row Row;
----------------
Any idea about the complexity of implementing this? It seems to me that most users either 1) already want a range, in which case this function can just return a pair of `{firstIndex, lastIndex}` and the caller can determine the range, or 2) want the info for a specific operation, in which case an OpIndex needs to be passed. I've not personally done anything with VLIW architectures but I'm assuming it's possible for different operations in the same instruction to have different source locations and even be in different inline frames, in which case it would be necessary to update a dozen or so callsites to pass an OpIndex through - seems like it would still be fairly simple in terms of code though.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:1091
     DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
-    Prologue.MaxOpsPerInst = MaxOpsPerInst;
+    Prologue.MaxOpsPerInst = 13;
     LT.setPrologue(Prologue);
----------------
It's not technically a part of supporting op-index, but might be a good idea to also include a `MinInstLength > 1` in this test - as mentioned above I think the calculation for the address offset is wrong if MinInstLength > 1, which I expect it would be for an actual VLIW line table.


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

https://reviews.llvm.org/D152536