[PATCH] D40339: Use getStoreSize() in various places instead of BitSize >> 3

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:597
   uint32_t getAlignInBits() const { return AlignInBits; }
+  uint64_t getSizeInBytes() const { return ((SizeInBits + 7) / 8); }
   uint32_t getAlignInBytes() const { return getAlignInBits() / CHAR_BIT; }
----------------
The getSizeInBytes() in other classes often truncates to number of whole bytes. Whereas for example getStoreSizeInBits() is doing a ceiling operation. Maybe this method should take a bool to indicate if ceiling or truncation is wanted in case of size in bits not being a multiple of the byte size. Making sure new uses of this method requires the author to choose between the two alternatives.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:816
 
-  uint64_t Size = BTy->getSizeInBits() >> 3;
+  uint64_t Size = BTy->getSizeInBytes();
   addUInt(Buffer, dwarf::DW_AT_byte_size, None, Size);
----------------
Do we know if this is correct? Or was the old behavior of truncating intentional?
Or are these constructTypeDIE  assuming that the size in bits is a multiple of 8? Maybe we even need to assert that the SizeInBits is a multiple of the byte size here (or inside the getSizeInBytes method) to ensure correct behavior (in case it is incorrect to both round up or down when the size in bits isn't a multiple of the byte size).

I think you need some test cases that proves that this is correct (if ceiling is needed). Or if you can't create such tests, nor prove that truncate is correct, then I think we should go for the asserts or llvm_unreachable. Such asserts would ensure that whenever, in the future, we end up here without having a multiple of the byte size, then we need to determine what the correct solution is.




https://reviews.llvm.org/D40339