[PATCH] D25914: Redo store splitting in CodeGenPrepare

[Wei Mi via llvm-commits]

Ping.

On Wed, Nov 30, 2016 at 10:17 AM, Wei Mi <wmi at google.com> wrote:
> On Mon, Nov 28, 2016 at 4:35 PM, Chandler Carruth via Phabricator
> <reviews at reviews.llvm.org> wrote:
>> chandlerc added inline comments.
>>
>>
>> ================
>> Comment at: lib/CodeGen/CodeGenPrepare.cpp:5300-5301
>> +                                const TargetLowering &TLI) {
>> +  unsigned HalfValBitSize =
>> +      DL.getTypeSizeInBits(SI.getValueOperand()->getType()) / 2;
>> +
>> ----------------
>> majnemer wrote:
>>> 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...
>> I think it is at least reasonable to start with type size == store size. Among other things, when that isn't true, the shifting and extension logic will be at least a little more complicated.
>>
>> Still, Wei, this might make sense as a follow-up patch to work on to extend this to handle more complicated merged stores. As a potentially useful set of examples you could look at the generated code for:
>>
>>   struct S {
>>     unsigned x : 27;
>>   };
>>
>>   std::pair<S, float> g1();
>>   std::pair<float, S> g2();
>>
>>   void f(std::pair<float, unsigned> &result) {
>>     auto p1 = g1();
>>     auto p2 = g2();
>>     result.first = p1.second + p2.first;
>>     result.second = p1.first + p2.second;
>>   }
>>
>> Or something similar... But I'm happy to do this as a follow-up generalization now that the correctness issues are addressed.
>>
>> That seem reasonable David?
>>
>
> I found the above testcase didn't require store split because
> "result.first =" and "result.second =" are already separate stores. I
> tried other tests and found it was not that easy to create a case from
> C/C++ source with non-power-of-2 size store which needs to be split,
> because of type promotion. But I will keep it in mind and prepare to
> do follow-up once I find such testcase.
>
> David and Chandler, do you think it is ok?
>
>>
>> ================
>> Comment at: test/CodeGen/X86/split-store.ll:100-119
>> +; CHECK-LABEL: int31_int31_pair
>> +; CHECK: andl $2147483647, %edi
>> +; CHECK: movl %edi, (%rdx)
>> +; CHECK: andl $2147483647, %esi
>> +; CHECK: movl %esi, 4(%rdx)
>> +define void @int31_int31_pair(i31 %tmp1, i31 %tmp2, i64* %ref.tmp) {
>> +entry:
>> ----------------
>> I would also include more negative tests:
>>
>> 1) non-symmetric merges
>> 2) non-power-of-two merged store sizes where type size == store size (i24, i48, etc)
>> 3) non-power-of-two merged store sizes where type size != store size (i14)
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D25914
>>
>>
>>