[PATCH][ARM] PR 49526: Add support for smmul,smmla,smmls instructions

[Yvan Roux]

On 25 November 2015 at 11:36, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> Hi Yvan,
>
>
> On 24/11/15 15:05, Yvan Roux wrote:
>>
>> Hi Kyrill,
>>
>> On 24 November 2015 at 14:31, Kyrill Tkachov <kyrylo.tkachov@arm.com>
>> wrote:
>>>
>>> Ping.
>>>
>>> Thanks,
>>> Kyrill
>>>
>>>
>>>
>>> On 13/11/15 11:50, Kyrill Tkachov wrote:
>>>>
>>>> Ping.
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg00686.html
>>>>
>>>> Thanks,
>>>> Kyrill
>>>>
>>>> On 06/11/15 17:05, Kyrill Tkachov wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> This patch introduces support for the smmul, smmla and smmls
>>>>> instructions
>>>>> that appear in armv6 architecture levels
>>>>> and higher. To quote the SMMUL description from the ARMARM:
>>>>> "Signed Most Significant Word Multiply multiplies two signed 32-bit
>>>>> values, extracts the most significant 32 bits of
>>>>> the result, and writes those bits to the destination register."
>>>>>
>>>>> The smmla and smmls are the multiply-accumulate and multiply-subtract
>>>>> extensions of that multiply.
>>>>> There also exists an smmulr variant that rounds the result rather than
>>>>> truncating it.
>>>>> However, when I tried adding patterns for those forms I got an LRA ICE
>>>>> that I was not able to figure out.
>>>>> I'll try to find a testcase for that, but in the meantime there's no
>>>>> reason to not have patterns for the
>>>>> non-rounding variants.
>>
>> I agree, but would be interested to give a look at the LRA issue when
>> you have a testcase.
>
>
> I've filed PR 68536 for the ICE I've been seeing. Any insight is, of course,
> welcome.

Ok great, I'll give a look.

>>>>> Bootstrapped and tested on arm-none-linux-gnueabihf.
>>>>> I've seen this trigger in quite a few places in SPEC2006 where it
>>>>> always
>>>>> made the code better.
>>>>>
>>>>> Ok for trunk?
>>
>> Patch looks good to me (but can't approved it), I just wonder if
>> adding a subreg_highpart_p predicate in emit-rtl.c would be useful,
>> looking at other usage of subreg_highpart_offset it doesn't seem to be
>> the case, but maybe for consistency with the existing
>> subreg_lowpart_p.
>
>
> Perhaps, we can put it there if there's much scope for its reuse in other
> parts of the compiler. I didn't put it there in the first place so as not to
> extend the review scope of the patch to the midend...

Yes it's what I thought, and I don't see other usage. Now on AArch64 I
see that the pattern used to generate smulh and umulh
(<su>muldi3_highpart), which are doing the same kind of operation in
the 64bit world, is doing it in a different manner (lshift and
truncate) and I'm not sure which approach is the best here.

Cheers,
Yvan

> Thanks for looking at this,
> Kyrill
>
>
>>
>> Cheers,
>> Yvan
>>
>>>>> Thanks,
>>>>> Kyrill
>>>>>
>>>>> 2015-11-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>
>>>>>      PR target/49526
>>>>>      * config/arm/arm.md (*mulsidi3si_v6): New pattern.
>>>>>      (*mulsidi3siaddsi_v6): Likewise.
>>>>>      (*mulsidi3sisubsi_v6): Likewise.
>>>>>      * config/arm/predicates.md (subreg_highpart_operator):
>>>>>      New predicate.
>>>>>
>>>>> 2015-11-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>>>
>>>>>      PR target/49526
>>>>>      * gcc.target/arm/pr49526_1.c: New test.
>>>>>      * gcc.target/arm/pr49526_2.c: Likewise.
>>>>>      * gcc.target/arm/pr49526_3.c: Likewise.
>>>>
>>>>
>