[PATCH] D25073: [DebugInfo]: preparation to implement DW_AT_alignment

[Adrian Prantl via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/IR/DIBuilder.h:460
     /// \param Decl        Reference to the corresponding declaration.
+    /// \param AlignInBits Variable alignment(or 0 if no alignment attr was
+    ///                      specified)
----------------
missing space


================
Comment at: include/llvm/IR/DIBuilder.h:461
+    /// \param AlignInBits Variable alignment(or 0 if no alignment attr was
+    ///                      specified)
     DIGlobalVariable *createGlobalVariable(DIScope *Context, StringRef Name,
----------------
extra space :-)


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1829
   unsigned Line;
+  uint64_t AlignInBits;
 
----------------
Sorry for noticing this so late:
Does this really need to be a 64bit integer, or would a 32-bit unsigned be sufficient?
A 32-bit unsigned could immediately follow the previous unsigned field und thus save 8 bytes.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1735
+  //   In this case Record size will be 10.
+  // We don't want to put any fake values into Record[1] and Record[9], we
+  // just encode the information that we have alignment value in Record[0],
----------------
It is more useful to future readers to explain what we did, rather than explain what alternatives we rejected ands why.
I would just enumerate the 3(?) different formats of this record and how to differentiate them.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1410
+      // which can't be done with bitfields. Thus we use FieldSize here.
+      uint64_t AlignInBits = FieldSize;
+      uint64_t AlignMask = ~(AlignInBits - 1);
----------------
Thanks!


https://reviews.llvm.org/D25073