On Tue, Dec 13, 2016 at 11:59:36AM +0000, Kyrill Tkachov wrote:
> Hi Naveen,
>
> On 13/12/16 11:51, Hurugalawadi, Naveen wrote:
> >Hi Kyrill,
> >
> >Thanks for reviewing the patch and your useful comments.
> >
> >>>looks good to me if it has gone through the normal required
> >>>bootstrap and testing, but I can't approve.
> >Bootstrapped and Regression Tested on aarch64-thunderx-linux.
> >
> >>>The rest of the MD file uses the term AdvSIMD. Also, the instrurction
> >>>is CNT rather than "pop count".
> >Done.
> >
> >>>__builtin_popcount takes an unsigned int, so this should be
> >>>scanning for absence of popcountsi2 instead?
> >Done.
> >
> >Please find attached the modified patch as per review comments
> >and let me know if its okay for Stage-1 or current branch.
>
> This looks much better, thanks.
> I still have a minor nit about the testcase.
>
> +long
> +foo1 (int x)
> +{
> + return __builtin_popcountl (x);
> +}
>
> On ILP32 systems this would still use the SImode patterns, so I suggest you
> use __builtin_popcountll and
> an unsigned long long return type to ensure you always exercise the 64-bit
> code.
>
> +
> +/* { dg-final { scan-assembler-not "popcount" } } */
>
>
> This looks ok to me otherwise, but you'll need an ok from the aarch64 folk.
I didn't see a follow-up patch posted with Kyrill's comments addressed?
Thanks,
James
