This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jonpa on Nov 22 2017, 3:04 AM.

Details

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.

Diff Detail

Event Timeline

jonpa created this revision.Nov 22 2017, 3:04 AM

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!

fhahn added a subscriber: fhahn.Nov 22 2017, 9:14 AM
bjope added a subscriber: bjope.Nov 23 2017, 8:57 AM
bjope added inline comments.Nov 23 2017, 10:14 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12623–12624

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

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 updated this revision to Diff 124205.Nov 24 2017, 6:52 AM
jonpa marked an inline comment as done.

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

jonpa added inline comments.Nov 24 2017, 6:56 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12623–12624

Seems right to me also - done.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp
100

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 added a comment.Nov 24 2017, 7:07 AM

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

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).

The MIPS part LGTM.

bjope accepted this revision.Nov 28 2017, 4:38 AM

LGTM

This revision is now accepted and ready to land.Nov 28 2017, 4:38 AM
jonpa added a comment.Nov 28 2017, 6:49 AM

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 added a comment.Nov 28 2017, 8:14 AM

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 updated this revision to Diff 124702.Nov 29 2017, 1:42 AM

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 ;-)

jonpa requested review of this revision.Nov 30 2017, 7:29 AM
jonpa edited edge metadata.
bjope added inline comments.Nov 30 2017, 3:17 PM
include/llvm/IR/DebugInfoMetadata.h
597 ↗(On Diff #124702)

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 ↗(On Diff #124702)

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 added a comment.Dec 1 2017, 5:49 AM

I have no idea about this...