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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12624
 
-  int64_t ElementSizeBytes = MemVT.getSizeInBits() / 8;
+  int64_t ElementSizeBytes = MemVT.getStoreSize();
   unsigned SizeInBits = NumStores * ElementSizeBytes * 8;
----------------
bjope wrote:
> 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.
Seems right to me also - done.


================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:100
+  unsigned SpillSize = ValueType.getStoreSize();
   assert((SpillSize * 8) == ValueType.getSizeInBits() && "Size not in bytes?");
 
----------------
bjope wrote:
> 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.
> 
> 
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...


https://reviews.llvm.org/D40339