Re: [RFC] [2/2] divmod transform: override expand_divmod_libfunc for ARM and add test-cases

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

On 27 July 2016 at 18:56, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
> On Wed, May 25, 2016 at 1:49 PM, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
>> On 23 May 2016 at 14:28, Prathamesh Kulkarni
>> <prathamesh.kulkarni@linaro.org> wrote:
>>> Hi,
>>> This patch overrides expand_divmod_libfunc for ARM port and adds test-cases.
>>> I separated the SImode tests into separate file from DImode tests
>>> because certain arm configs (cortex-15) have hardware div insn for
>>> SImode but not for DImode,
>>> and for that config we want SImode tests to be disabled but not DImode tests.
>>> The patch therefore has two target-effective checks: divmod and divmod_simode.
>>> Cross-tested on arm*-*-*.
>>> Bootstrap+test on arm-linux-gnueabihf in progress.
>>> Does this patch look OK ?
>> Hi,
>> This version adds couple of more test-cases and fixes typo in
>> divmod-3-simode.c, divmod-4-simode.c
>>
>> Thanks,
>> Prathamesh
>>>
>>> Thanks,
>>> Prathamesh
>
> From the patch (snipped out unnecessary parts)
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 201aeb4..3bbf11b 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
>
> <snip>
>
> +  gcc_assert (quotient);
> +  gcc_assert (remainder);
> +
>
> There's a trailing white space here.
>
> +  *quot_p = quotient;
> +  *rem_p = remainder;
> +}
>
>
>
> +# For ARM configs defining __ARM_ARCH_EXT_IDIV__, disable
> divmod_simode test-cases
>
> Very unhelpful comment ...
>
> For versions of the architecture where there exists a DIV instruction,
> the divmod helper function is not used, disable the software divmod
> optimization.
>
>
> +
> +proc check_effective_target_arm_divmod_simode { } {
> +    return [check_no_compiler_messages arm_divmod assembly {
> +       #ifdef __ARM_ARCH_EXT_IDIV__
> +       #error has div insn
> +       #endif
> +       int i;
> +    }]
> +}
> +
> +proc check_effective_target_divmod { } {
>
> Missing comment above.
>
> +    #TODO: Add checks for all targets that have either hardware divmod insn
> +    # or define libfunc for divmod.
> +    if { [istarget arm*-*-*]
> +        || [istarget x86_64-*-*] } {
> +       return 1
> +    }
> +    return 0
> +}
>
>
>
>
>
> The new helper functions need documentation in doc/sourcebuild.texi
>
> Please repost with the doc changes, otherwise this is OK from my side.
Hi Ramana,
Thanks for the review, I have updated the patch
with your suggestions.
Cross-tested on arm*-*-*.

I came across following issue while bootstrapping
on armv8l-unknown-linux-gnueabihf:
All the divmod-*-simode.c tests which have
/* { dg-require-effective-target divmod_simode } */
appear UNSUPPORTED.
That's because this config appears to define
__ARM_ARCH_EXT_IDIV__ however idiv appears not to be present.

For instance __aeabi_div is called to perform
division for the following test-case:
int f(int x, int y)
{
  int r = x / y;
  return r;
}

Compiling with -O2:
f:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
push    {r4, lr}
bl      __aeabi_idiv
pop     {r4, pc}

I assumed if __ARM_ARCH_EXT_IDIV was defined, then
there should have been idiv instead of call to __aeabi_div
or am I missing something ?

Um I had configured with --with-tune=cortex-a9. Is that incorrect for
armv8l-unknown-linux-gnueabihf ?

xgcc -v:
Using built-in specs.
COLLECT_GCC=armhf-bootstrap-build/gcc/xgcc
Target: armv8l-unknown-linux-gnueabihf
Configured with: ../gcc/configure --enable-languages=c,c++,fortran
--with-arch=armv8-a --with-fpu=neon-fp-armv8 --with-float=hard
--with-mode=thumb --enable-multiarch --with-tune=cortex-a9
--disable-multilib
Thread model: posix
gcc version 7.0.0 20160727 (experimental) (GCC)

Thanks,
Prathamesh
>
> Thanks,
> Ramana

Attachment: divmod-1-arm.diff
Description: Text document