[PATCH] D25914: Redo store splitting in CodeGenPrepare

Wei Mi via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 28 16:10:34 PST 2016


On Mon, Nov 28, 2016 at 3:47 PM, David Majnemer via Phabricator
<reviews at reviews.llvm.org> wrote:
> 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...
>

My original assumption is we cannot have i63 type store which would
pass the pattern match check. That may not be fully correct because we
may create a very special case breaking the assumption. So I think
Chandler's suggestion to explicitly check "type store size == the type
size" is a good idea and will guanrantee common cases work as
expected.

>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D25914
>
>
>


More information about the llvm-commits mailing list