ping?
On 22 May 2014 11:08, Charles Baylis <charles.bay...@linaro.org> wrote:
> On 1 May 2014 16:41, Richard Earnshaw <rearn...@arm.com> wrote:
>> I think really, you've got three independent changes here:
>
> Version 2 of this patch series is now a 9 patch series which addresses
> most of the following. Exceptions discussed below.
>
>> 1) Optimize the prologue/epilogue sequences when ldrd is available.
>> 2) Replace the call to __gnu_ldivmod_helper with __udivmoddi4
>
> I assume you mean __gnu_uldivmod_helper here, as __gnu_ldivmod_helper
> performs signed division and can't be directly replaced with the
> unsigned division performed by __udivmoddi4.
>
>> 3) Optimize the code to __aeabi_ldivmod.
>
> Converting to call __udivmoddi4, fixing up signedness of operands and
> results and optimisation are all one change.
>
>> Ideally, therefore, this is a three patch series, but it's then missing
>> a few bits.
>>
>> 4) Step 2 can also be trivially applied to bpabi-v6m.S as well, since
>> it's a direct swap of one function for another (unless I've misread the
>> changes, I think the ABI of the two helper functions are the same).
>
> For __aeabi_uldivmod this is true. For __aeabi_ldivmod this is not
> trivial as the signedness fix-ups must be written.
>
>> 5) Step 4 then makes __gnu_ldivmod_helper in bpabi.c a dead function
>> which can be deleted. This is good because currently pulling in either
>> 64-bit division function causes both these helper functions to be pulled
>> in and thus the whole of the 64-bit div-mod code for both signed and
>> unsigned values. That's particularly unfortunate for ARMv6m class
>> devices as that's potentially a lot of redundant code.
>
> Similarly, __gnu_uldivmod_helper not __gnu_ldivmod_helper.
>
> I've included two patches which do the trivial steps for the unsigned case.
>
>>
>> Finally, I know this was the original code, but the complete lack of
>> comments in this code made reviewing even the trivial parts a complete
>> nightmare -- it took me half an hour before I remembered that
>> __udivmoddi4 took three parameters, the third of which was on the stack:
>> thus the messing around with sp/ip in the prologue wasn't just trivial
>> padding but a necessary part of the function. Please could you add, at
>> least some short comments clarifying the register disposition on input
>> and what that prologue code is up to...
>
> Done.
>
>> Finally, how was this code tested?
>
> It has been built and "make check" has been run with no regressions on:
> arm-unknown-linux-gnueabihf --with-mode=thumb --with-arch=armv7-a
> arm-unknown-linux-gnueabihf --with-mode=arm --with-arch=armv7-a
> arm-unknown-linux-gnueabi --with-mode=arm --with-arch=armv5te
> arm-unknown-linux-gnueabi --with-mode=arm --with-arch=armv4t
>
> I have also run a simple test harness which checks the result of
> several 64 bit division operations where gcc has been built with the
> above configurations.
>
> I am not currently set up with a way to test v6M, so those parts aren't
> tested.
>
>> Anyway, some additional comments below:
>>
>> Don't repeat the function name for multiple tweaks to the same function;
>> as mentioned above, if these are really separate changes they should be
>> in separate submissions. Mixing unrelated changes just makes the
>> reviewing step that much harder.
>
> Done.
>
>
>>> + strd ip,lr, [sp, #-16]!
>>
>> Space after comma.
>
> Done
>
>> Also, since you've essentially rewritten the entire function, can you
>> please also reformat them to follow the coding style of the rest of the
>> file: namely "<tab>OP<tab>operands".
>
> Done
>
>>> #else
>>> + sub sp, sp, #8
>>> do_push {sp, lr}
>>> #endif
>>
>> Please add a comment that the value at *sp is the address of the the
>> slot for the remainder.
>
> Done
>>> +#if defined(__thumb2__) && CAN_USE_LDRD
>>> + sub ip, sp, #8
>>> + strd ip,lr, [sp, #-16]!
>>
>> Space after comma.
>
> Done
>
>>> #else
>>> + sub sp, sp, #8
>>> do_push {sp, lr}
>>> #endif
>>> + cmp xxh, #0
>>> + blt 1f
>>> + cmp yyh, #0
>>> + blt 2f
>>> +
>>> +98: cfi_push 98b - __aeabi_ldivmod, 0xe, -0xc, 0x10
>>
>> The CFI push should really precede your conditional tests, it relates to
>> the do_push expression.
>
> Done.
>
>>> + bl SYM(__udivmoddi4) __PLT__
>>> + ldr lr, [sp, #4]
>>> +#if CAN_USE_LDRD
>>> + ldrd r2, r3, [sp, #8]
>>> + add sp, sp, #16
>>> +#else
>>> + add sp, sp, #8
>>> + do_pop {r2, r3}
>>> +#endif
>>
>> You're missing a CFI pop, which is needed when the values on the stack
>> go out of scope.
>
> The existing code doesn't do this. Since there are multiple exit
> points from the optimised function the existing cfi_* macros aren't
> sufficient (there is no cfi_save_state/cfi_restore_state), so I have
> included a patch which uses the gas .cfi_* directives. This may be
> interesting on non-DWARF or non-ELF platforms, if any are still
> supported .
>
>>> + RET
>>> +1: /* xxh:xxl is negative */
>>> + rsbs xxl, xxl, #0
>>
>> We're using unified syntax, so NEGS is preferable.
>
> Done
>
>>> + sbc xxh, xxh, xxh, lsl #1
>>
>> Worthy of a comment, Thumb2 has no RSC instruction, so use X - 2X.
>
> Done
>
>>> + cmp yyh, #0
>>> + blt 3f
>>> +98: cfi_push 98b - __aeabi_ldivmod, 0xe, -0xc, 0x10
>>
>> This CFI push looks wrong. You've already pushed things earlier. On
>> the other hand, you should save the state before the CFI pop above, so
>> that you can restore the state again for the next (ie this) block of code.
>
> Done (see above)
>
>>> +98: cfi_push 98b - __aeabi_ldivmod, 0xe, -0xc, 0x10
>>> + bl SYM(__udivmoddi4) __PLT__
>>> + ldr lr, [sp, #4]
>>> +#if CAN_USE_LDRD
>>> + ldrd r2, r3, [sp, #8]
>>> + add sp, sp, #16
>>> +#else
>>> + add sp, sp, #8
>>> + do_pop {r2, r3}
>>> +#endif
>>> + rsbs yyl, yyl, #0
>>> + sbc yyh, yyh, yyh, lsl #1
>>> + RET
>>>
>>> #endif /* L_aeabi_ldivmod */
>>>
>>
>> You use the LDRD vs do_pop sequence identically several times. To avoid
>> a lot of ifdefs, it might be worth considering a macro for this idiom to
>> reduce the overall amount of conditionalized code.
>
> Done.
>
>
> The updated patch series is attached. Hopefully, patches 1 through 6
> are now ready. Patches 7 through 9 can be dropped if necessary.
>
>
>
>
> 0001-Whitespace.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_uldivmod): Fix whitespace.
> (__aeabi_ldivmod): Fix whitespace.
>
>
>
> 0002-Add-comments.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_uldivmod, __aeabi_ldivmod): Add comment
> describing register usage on function entry and exit.
>
>
>
> 0003-Optimise-__aeabi_uldivmod-stack-manipulation.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_uldivmod): Optimise stack pointer
> manipulation.
>
>
>
> 0004-Optimise-__aeabi_uldivmod.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_uldivmod): Perform division using call
> to __udivmoddi4.
>
>
>
> 0005-Optimise-__aeabi_ldivmod-stack-manipulation.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_ldivmod): Optimise stack manipulation.
>
>
>
> 0006-Optimise-__aeabi_ldivmod.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_ldivmod): Perform division using
> __udivmoddi4, and fixups for negative operands.
>
>
>
> 0007-Fix-cfi-annotations.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.S (__aeabi_ldivmod, __aeabi_uldivmod,
> push_for_divide, pop_for_divide): Use .cfi_* directives for DWARF
> annotations. Fix DWARF information.
>
>
>
> 0008-Use-__udivmoddi4-for-v6M-aeabi_uldivmod.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi-v6m.S (__aeabi_uldivmod): Perform division using
> __udivmoddi4.
>
>
>
> 0009-Remove-__gnu_uldivmod_helper.patch
>
> 2014-05-22 Charles Baylis <charles.bay...@linaro.org>
>
> * config/arm/bpabi.c (__gnu_uldivmod_helper): Remove.
