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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

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



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


https://reviews.llvm.org/D40339