[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:512
+
+/// EmitValue - Emit base type reference.
+///
----------------
under the current style you may drop the duplicate doxygen comments con the function implementations.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:516
+  uint64_t Offset = CU->ExprRefedBaseTypes[Index].Die->getOffset();
+  assert(Offset < (1ULL << (ULEB128PadSize * 7)) && "Offset wont fit");
+  AP->EmitULEB128(Offset, nullptr, ULEB128PadSize);
----------------
what happens when this assertion isn't met in a release compiler? Is that a purely hypothetical scenario?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1198
+  // iterating backwards and inserting to the front of CU child list.
+  for (std::vector<BaseTypeRef>::reverse_iterator
+       I = ExprRefedBaseTypes.rbegin(), E = ExprRefedBaseTypes.rend();
----------------
`for (auto &I : reverse(ExprRefedBaseTypes))`


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:403
+        // later.
+        unsigned I = 0, E = CU.ExprRefedBaseTypes.size();
+        for (; I != E; ++I)
----------------
Why not use a SmallDenseSet for CU.ExprRefedBaseTypes?


================
Comment at: test/DebugInfo/Generic/convert-inlined.ll:32
+; DW5-CHECK-NEXT:                 DW_AT_decl_line	(1)
+; DW5-CHECK-NEXT:                 DW_AT_location	(DW_OP_addrx 0x0, DW_OP_deref, DW_OP_convert 0x1e, DW_OP_convert 0x22, DW_OP_stack_value)
+; DW5-CHECK-EMPTY:
----------------
Sorry about the extra work this causes, but it would really pay off to also have a separate peraratory commit that adds dwarfdump support for DW_OP_convert so this is printed as `DW_OP_convert (0x000022 "DW_ATE_signed_32")` as well as dwarfdump --verify support that ensures that the offset actually points to a type DIE.


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

https://reviews.llvm.org/D56587