[PATCH] D25914: Redo store splitting in CodeGenPrepare

[David Majnemer via Phabricator via llvm-commits]

majnemer added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5300-5301
+                                const TargetLowering &TLI) {
+  unsigned HalfValBitSize =
+      DL.getTypeSizeInBits(SI.getValueOperand()->getType()) / 2;
+
----------------
chandlerc wrote:
> wmi wrote:
> > majnemer wrote:
> > > wmi wrote:
> > > > majnemer wrote:
> > > > > This looks wrong. Shouldn't it be `getTypeStoreSizeInBits` instead of `getTypeSizeInBits`?
> > > > They are the same here because the type of the store value must have power of 2 size if it is a merged store. But you remind me to add some testcases: @int31_float_pair, @int15_float_pair, @int7_float_pair added in the testcase. 
> > > But aren't i1, i2 and i4 powers of two?
> > > 
> > > If we stored an i1, `HalfValBitSize` would be zero which sounds problematic.
> > The store val will at least be i2 because it is a merged val from two smaller vals. 
> > 
> > If the store val is an i2 which are combined from an {i1, i1} pair, and we use getTypeStoreSizeInBits to compute the HalfValBitSize, the HalfValBitSize will be 4. It means the split store size will be 4 bits. It is not what we expect. We expect the type of split store is i1. 
> > 
> > I cannot add add an i1_i1_pair test because now the target query will return false for int pair. But I have verified that the i1_i1_pair test worked correctly to use getTypeSizeInBits by disabling the target query temporarily.
> > 
> > 
> I don't think storing i1s makes any sense here.
> 
> I think you should add a check that the type store size == the type size both before and after splitting and not split unless that is satisfied, regardless of what the target says.
> 
> And to make this easier to test, I suggest adding a flag that forces us to split everything we can split. Otherwise covering interesting inputs is too target dependent.
I'm still not sure why we would want to use TypeSizeInBits instead of the store size...
What if the type is an i63? We would actually store 8 bytes...


Repository:
  rL LLVM

https://reviews.llvm.org/D25914