[PATCH] D52295: [DebugInfoMetadata] Added support to generate packed_decimal encoding related dwarf info.

[Paul Robinson via Phabricator via llvm-commits]

probinson added subscribers: llvm-commits, probinson.
probinson added a comment.

+llvm-commits

clang-format-diff is your friend.

Remind me of your use case?  Clang does not support any decimal types, although there is work in progress to support scaled binary.



================
Comment at: include/llvm/BinaryFormat/Dwarf.def:702
 
+//DWARF decimal sign
+HANDLE_DW_DS(0x01, unsigned)
----------------
Need a space after the //


================
Comment at: include/llvm/BinaryFormat/Dwarf.def:707
+HANDLE_DW_DS(0x04, leading_separate)
+HANDLE_DW_DS(0x05, trailing_separate)
+
----------------
aprantl wrote:
> Thanks! Looks like none of these were introduced recently so we can get away with not encoding the version number this was introduced in.
Right the decimal stuff was all added in DWARF 3 and I'm pretty sure has not changed since.  As these are enums used in an attribute that is correctly identified as v3, I don't think these need a version.


================
Comment at: include/llvm/IR/DIBuilder.h:203
+    /// \param SizeInBits  Size of the type.
+    /// \param Encoding    DWARF encoding code, e.g., dwarf::DW_ATE_float.
+    /// \param DigitCount  Digits count in packed decimal type
----------------
I don't know that DW_ATE_float is an appropriate example here.


================
Comment at: include/llvm/IR/DIBuilder.h:204
+    /// \param Encoding    DWARF encoding code, e.g., dwarf::DW_ATE_float.
+    /// \param DigitCount  Digits count in packed decimal type
+    /// \param DecimalSign decimal sign
----------------
Does it have to be "packed" decimal type?  That may be your use case but as you are passing the encoding as a parameter, I think this would work for all the decimal types.


================
Comment at: include/llvm/IR/DIBuilder.h:206
+    /// \param DecimalSign decimal sign
+    /// \param DeimalScale decimal scale
+    /// \param Flags       Optional DWARF attributes, e.g., DW_AT_endianity.
----------------
DecimalScale


================
Comment at: lib/AsmParser/LLParser.cpp:4335
+///                    encoding: DW_ATE_encoding, flags: 0, pic: "S999V999",
+///                    digits: 4, sign: DW_DS_leading_overpunch, scale: -2, )
 bool LLParser::ParseDIBasicType(MDNode *&Result, bool IsDistinct) {
----------------
I'd prefer the example to be self-consistent (digits/scale matching the pic string).


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1500
+  // except the `scale` attribute which is integer and may have valid zero
+  // value, to mark its validity we add one more non-zero record before it
+  if (N->hasDecimalInfo()) {
----------------
LLVM style uses full sentences in comments (with proper capitalization and punctuation).


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:753
+
+    if (const auto &scale= BTy->getDecimalScale())
+      addSInt(Buffer, dwarf::DW_AT_decimal_scale, None, *scale);
----------------
Space before the =.


https://reviews.llvm.org/D52295