Use getStoreSize() in various places instead of BitSize >> 3

[jonpa]

Summary

A bug (https://bugs.llvm.org/show_bug.cgi?id=35366) in DAGCombiner was discovered where loads/stores of i1 got the size of 0, due to the use of 'BitSize >> 3'. The proper way to get the memory access size is to use getStoreSize().

Since this is found also in other places, I have tried to fix a few more of them where it looks obvious.

Please review for correctness.

[uabelho]

As a side note: This also helps people like myself that live with an out-of-tree target with
non-8bit bytes. We've changed getStoreSize() so it takes the byte size into account, so
using that instead of all these "/ 8" and ">> 3" all over the place makes life easier for us as
well. Thanks for that Jonas!

[bjope]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12624 ↗

(On Diff #123895)

As far as I can see all uses of ElementSizeBytes in this method is doing "ElementSizeBytes * 8".
So I gues we can replace this by a

int64_t ElementSizeBits = MemVT.getStoreSizeInBits();

and get rid of all those multiplications by 8.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

100 ↗

(On Diff #123895)

I wouldn't mind if this assert is changed into:

assert(ValueType.getStoreSizeInBits() == ValueType.getSizeInBits() && "Size not equal to store size");

Or is the assert aiming at checking if ValueType is a multiple of the byte size, then it should be

assert(ValueType.isByteSized() && "Size not in bytes?");

but despite the current assert string I doubt that is what we want to verify.

[jonpa]

MergeStoresOfConstantsOrVecElts() updated to use getStoreSizeInBits() per review.

[jonpa]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

12624 ↗

(On Diff #123895)

Seems right to me also - done.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

100 ↗

(On Diff #123895)

I'm not exactly sure either - unless someone confirms this, perhaps this can be changed at a later point after we get these fixes in...

[bjope]

I have no further comments. And it looks good from our out-of-tree-targets perspective.
In many situations I think this is NFC. For example the consecutive load/store optimizations have other checks verifying that the involved types are byte sized, so the end result will be the same.
But I haven't really reviewed the MIPS/Hexagon specific parts, and there are no test cases showing that something is more correct (I'm not sure all changes are NFC).

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

100 ↗

(On Diff #123895)

Sure, I don't want this to be a stopper for your patch so you may ignore it. It was just something I discovered when reviewing (not directly connected to your patch).

[sdardis]

The MIPS part LGTM.

[bjope]

LGTM

[jonpa]

Thanks for review. r319173.

BTW,

What about all the cases using Type* , like 'ByteSize = Ty->getSizeInBits() / 8;'... These should also be fixed, I presume, but I don't see a getStoreSize() method or similar in Type...

Also, with

diff --git a/include/llvm/IR/DebugInfoMetadata.h b/include/llvm/IR/DebugInfoMetadata.h
index c515f6d..a8aa195 100644
--- a/include/llvm/IR/DebugInfoMetadata.h
+++ b/include/llvm/IR/DebugInfoMetadata.h
@@ -594,6 +594,7 @@ public:
   unsigned getLine() const { return Line; }
   uint64_t getSizeInBits() const { return SizeInBits; }
   uint32_t getAlignInBits() const { return AlignInBits; }
+  uint64_t getSizeInBytes() const { return (SizeInBits < 8 ? 1 : SizeInBits >> 3); }
   uint32_t getAlignInBytes() const { return getAlignInBits() / CHAR_BIT; }
   uint64_t getOffsetInBits() const { return OffsetInBits; }
   DIFlags getFlags() const { return Flags; }
diff --git a/lib/CodeGen/AsmPrinter/DwarfUnit.cpp b/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
index 911e462..8829886 100644
--- a/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
+++ b/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
@@ -813,14 +813,14 @@ void DwarfUnit::constructTypeDIE(DIE &Buffer, const DIBasicType *BTy) {
   addUInt(Buffer, dwarf::DW_AT_encoding, dwarf::DW_FORM_data1,
           BTy->getEncoding());
 
-  uint64_t Size = BTy->getSizeInBits() >> 3;
+  uint64_t Size = BTy->getSizeInBytes();
   addUInt(Buffer, dwarf::DW_AT_byte_size, None, Size);
 }
 
 void DwarfUnit::constructTypeDIE(DIE &Buffer, const DIDerivedType *DTy) {
   // Get core information.
   StringRef Name = DTy->getName();
-  uint64_t Size = DTy->getSizeInBits() >> 3;
+  uint64_t Size = DTy->getSizeInBytes();
   uint16_t Tag = Buffer.getTag();
 
   // Map to main type, void will not have a type.
@@ -907,7 +907,7 @@ void DwarfUnit::constructTypeDIE(DIE &Buffer, const DICompositeType *CTy) {
   // Add name if not anonymous or intermediate type.
   StringRef Name = CTy->getName();
 
-  uint64_t Size = CTy->getSizeInBits() >> 3;
+  uint64_t Size = CTy->getSizeInBytes();
   uint16_t Tag = Buffer.getTag();
 
   switch (Tag) {

I got

Failing Tests (3):
    LLVM :: DebugInfo/Generic/namespace.ll
    LLVM :: DebugInfo/Generic/tu-composite.ll
    LLVM :: DebugInfo/X86/sret.ll

Could this be an indicator that something *should* be handled here, or is this a bad fix for some reason?

[bjope]

In D40339#937625, @jonpa wrote:

Thanks for review. r319173.

BTW,

What about all the cases using Type* , like 'ByteSize = Ty->getSizeInBits() / 8;'... These should also be fixed, I presume, but I don't see a getStoreSize() method or similar in Type...

Also, with

diff --git a/include/llvm/IR/DebugInfoMetadata.h b/include/llvm/IR/DebugInfoMetadata.h
index c515f6d..a8aa195 100644
--- a/include/llvm/IR/DebugInfoMetadata.h
+++ b/include/llvm/IR/DebugInfoMetadata.h
@@ -594,6 +594,7 @@ public:
   unsigned getLine() const { return Line; }
   uint64_t getSizeInBits() const { return SizeInBits; }
   uint32_t getAlignInBits() const { return AlignInBits; }
+  uint64_t getSizeInBytes() const { return (SizeInBits < 8 ? 1 : SizeInBits >> 3); }

Your way of implementing getSizeInBytes() is having some flaws:

1. for SizeInBits==0 it will return 1. <--- Seems wrong to me.
2. for SizeInBits > 8, but not a multiple of the byte size it rounds down (whereas it rounds up for < 8). <--- Inconsistent.

I think that a correct way of doing it would be

uint64_t getSizeInBytes() const { return (SizeInBits + (ByteSizeInBits - 1) / ByteSizeInBits); }

where "ByteSizeInBits" is 8 for all in-tree-targets.

[jonpa]

Your way of implementing getSizeInBytes() is having some flaws

Aah, sorry - seems I was just handling i1. Thanks for the suggestion. Now all tests are still green...

(Sorry for hard-coding the ByteSizeInBits to 8 once again, but it should save you two instances in total ;-)

[bjope]

include/llvm/IR/DebugInfoMetadata.h

597

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.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

816

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.

[jonpa]

I have no idea about this...

include/llvm/IR/DebugInfoMetadata.h

 public:
   TempDIType clone() const {
     return TempDIType(cast<DIType>(MDNode::clone().release()));
   }
 
   unsigned getLine() const { return Line; }
   uint64_t getSizeInBits() const { return SizeInBits; }
   uint32_t getAlignInBits() const { return AlignInBits; }
+  uint64_t getSizeInBytes() const { return ((SizeInBits + 7) / 8); }
   uint32_t getAlignInBytes() const { return getAlignInBits() / CHAR_BIT; }
   uint64_t getOffsetInBits() const { return OffsetInBits; }
   DIFlags getFlags() const { return Flags; }
 
   DIScopeRef getScope() const { return DIScopeRef(getRawScope()); }
   StringRef getName() const { return getStringOperand(2); }
 
 

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

void DwarfUnit::constructTypeDIE(DIE &Buffer, const DIBasicType *BTy) {
 
   // An unspecified type only has a name attribute.
   if (BTy->getTag() == dwarf::DW_TAG_unspecified_type)
     return;
 
   addUInt(Buffer, dwarf::DW_AT_encoding, dwarf::DW_FORM_data1,
           BTy->getEncoding());
 
-  uint64_t Size = BTy->getSizeInBits() >> 3;
+  uint64_t Size = BTy->getSizeInBytes();
   addUInt(Buffer, dwarf::DW_AT_byte_size, None, Size);
 }
 
 void DwarfUnit::constructTypeDIE(DIE &Buffer, const DIDerivedType *DTy) {
   // Get core information.
   StringRef Name = DTy->getName();
-  uint64_t Size = DTy->getSizeInBits() >> 3;
+  uint64_t Size = DTy->getSizeInBytes();
   uint16_t Tag = Buffer.getTag();
 
   // Map to main type, void will not have a type.
   const DIType *FromTy = resolve(DTy->getBaseType());
   if (FromTy)
     addType(Buffer, FromTy);
 
   // Add name if not anonymous or intermediate type.
void DwarfUnit::constructTypeDIE(DIE &Buffer, const DISubroutineType *CTy) {
   if (CTy->isRValueReference())
     addFlag(Buffer, dwarf::DW_AT_rvalue_reference);
 }
 
 void DwarfUnit::constructTypeDIE(DIE &Buffer, const DICompositeType *CTy) {
   // Add name if not anonymous or intermediate type.
   StringRef Name = CTy->getName();
 
-  uint64_t Size = CTy->getSizeInBits() >> 3;
+  uint64_t Size = CTy->getSizeInBytes();
   uint16_t Tag = Buffer.getTag();
 
   switch (Tag) {
   case dwarf::DW_TAG_array_type:
     constructArrayTypeDIE(Buffer, CTy);
     break;
   case dwarf::DW_TAG_enumeration_type:
     constructEnumTypeDIE(Buffer, CTy);