[PATCH] [ARM] fix some ld/st instruction with wirteback

[Jiangning Liu]

Hi Renato,

Thanks for your review!

If you look at the test cases I added, you would be able to understand what
the problem is.

I added 4 new tests cases, and they are for writeback versions of
vst3.v1i64, vst4.v1i64, vld3.v1i64 and vld4.v1i64. Originally they all
trigger assertion failures.

For example, before my code change, we can only generate "vld1.64    {d16,
d17, d18}, [r1:64]", and would fail to generate "vld1.64    {d16, d17,
d18}, [r1:64]!"

For llvm.arm.neon.vld3.v1i64 plus 3*64 offset writeback, we can use vld1
with writeback to implement the same functionality, because ARM instruction
vld1 accept 64-bit vector list.

If you look at function ARMDAGToDAGISel::SelectVLD, you may notice there
are some comments highlighting some hacking code as below,

// We use a VST1 for v1i64 even if the pseudo says vld2/3/4, so
// check for that explicitly too. Horribly hacky, but temporary.

I don't know when this was introduced, and unfortunately this introduced
bugs, because ARM::VST1q64wb_fixed is not a writeback version SDNode at
all. This is why you can see the SDNode changes in ARMDAGToDAGISel::Select
as well. And in the same way I changed ARM::VLD1q64wb_fixed to
ARM::VLD1d64QPseudoWB_fixed.

In order to make register allocator and scheduler works to correctly
allocate vector list, we have to use Pseudo ISD node. This is why we have a
mapping from VLD1d64QPseudoWB_fixed to ARM::VLD1d64Qwb_fixed in file
ARMExpandPseudoInsts.cpp.

Also, there are some other FIXME comments in ARMDAGToDAGISel::SelectVLD as
well like below,

// FIXME: VLD1/VLD2 fixed increment doesn't need Reg0. Remove the reg0
// case entirely when the rest are updated to that form, too.

This is why we have to introduce a mapping from _fixed version to _register
version in function getVLDSTRegisterUpdateOpcode. I think this mapping is
really tricky. To avoid the inconsistency between
getVLDSTRegisterUpdateOpcode and the code of adding Reg0 operand in
ARMDAGToDAGISel::SelectVLD, I introduced two new function isVLDfixed and
isVSTfixed to cover all _fixed ISDNode. they are used in two places. One is
for assertion in getVLDSTRegisterUpdateOpcode and the other is the
condition code of adding Reg0 in ARMDAGToDAGISel::SelectVLD.

Hopefully, you can understand what the problem is now.

Thanks,
-Jiangning



2014/1/15 Renato Golin <renato.golin at linaro.org>

> Hi Jiangning,
>
> Can you elaborate on what the problem was and how you fixed? I have to say
> I'm a bit lost just by looking at the code.
>
> Also, you seem to have changed the file mode to executable, you might want
> to rectify that before commit. ;)
>
> cheers,
> --renato
>
>
> On 13 January 2014 10:08, Jiangning Liu <liujiangning1 at gmail.com> wrote:
>
>> Hi,
>>
>> Attached patch is to fix the assertion failures exposed by some ld/st
>> instructions requiring writeback. Review, please!
>>
>> --
>> Thanks,
>> -Jiangning
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>


-- 
Thanks,
-Jiangning
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140115/955713f4/attachment.html>