This is the mail archive of the newlib@sourceware.org mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/3] arm: Remove optpld macro


First, sorry to reply to my own e-mail, I wasn't previously subscribed to the
newlib mailing list and got dropped from the CC list, I've quoted relevant
bits from the web archive replies below:

> The macro definition is not an assembly block as such, but rather just
> that: a macro definition which is supposed to be evaluated at compile
> (or assemble) time.  To reorder it on the source level seems completely
> out of the scope of LTO.

My understanding is that every "top-level" (global) invocation of the asm()
directive is converted into its own object and evaluation independently by
LTO. It doesn't matter that they're in the same file, LTO simply sees an
unordered global list of objects that do or do not reference each other.
[Googling "lto top level asm" was very informative. Seems lots of other
projects had similar issues]

The asm() block that was #include'd isn't referenced by anything so it's
dropped (which makes sense for what LTO is trying to do).

> I'm also confused because the patch is talking about link-time optimization,
> not compilation. But Pat says it causes a compilation failure. So then how
> is this related to LTO, even?

Sorry, that's a bit of sloppy language on my part. newlib with LTO compiles
just fine without this patch, but during the final link of the end application,
this becomes an issue.

> In addition, I have another question/issue. Sure, it is easy to modify the Newlib code, itself, to avoid this macro, but how can we know that there is not any user code which uses it that would now fail when it has been removed? This is a higher-level include file that users might have used; it is not a lower-level, newlib-internal file, it does not seem. That is, is it OK for compatibility reasons to just remove it (especially without a note as to why it is dangerous)?

I can't speak to that, but I can say that the current #include a macro asm
block is definitely does not work and will break with LTO, which I imagine
will become more common.

On Wed, Jan 11, 2017 at 11:50 PM, Pat Pannuto <pat.pannuto@gmail.com> wrote:
> LTO can re-order top-level assembly blocks, which can cause this
> macro definition to appear after its use (or not at all), causing
> compilation failures. As the macro has very few uses, simply removing
> it by inlining is a simple fix.
>
> n.b. one of the macro invocations in strlen-stub.c was already
> guarded by the relevant #define, so it is simply converted directly
> to a pld
> ---
>  newlib/libc/machine/arm/arm_asm.h     | 13 -------------
>  newlib/libc/machine/arm/strcpy.c      |  8 ++++++--
>  newlib/libc/machine/arm/strlen-stub.c |  8 +++++---
>  3 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/newlib/libc/machine/arm/arm_asm.h b/newlib/libc/machine/arm/arm_asm.h
> index 1bb5edb23..bf18c0a03 100644
> --- a/newlib/libc/machine/arm/arm_asm.h
> +++ b/newlib/libc/machine/arm/arm_asm.h
> @@ -71,12 +71,6 @@
>  #endif
>  .endm
>
> -.macro optpld  base, offset=#0
> -#if defined (_ISA_ARM_7)
> -       pld     [\base, \offset]
> -#endif
> -.endm
> -
>  #else
>  asm(".macro  RETURN    cond=\n\t"
>  #if defined (_ISA_ARM_4T) || defined (_ISA_THUMB_1)
> @@ -86,13 +80,6 @@ asm(".macro  RETURN  cond=\n\t"
>  #endif
>      ".endm"
>      );
> -
> -asm(".macro optpld     base, offset=#0\n\t"
> -#if defined (_ISA_ARM_7)
> -    "pld       [\\base, \\offset]\n\t"
> -#endif
> -    ".endm"
> -    );
>  #endif
>
>  #endif /* ARM_ASM__H */
> diff --git a/newlib/libc/machine/arm/strcpy.c b/newlib/libc/machine/arm/strcpy.c
> index b2e3f5177..b90d5cf73 100644
> --- a/newlib/libc/machine/arm/strcpy.c
> +++ b/newlib/libc/machine/arm/strcpy.c
> @@ -44,7 +44,9 @@ strcpy (char* dst, const char* src)
>    asm (
>  #if !(defined(__OPTIMIZE_SIZE__) || defined (PREFER_SIZE_OVER_SPEED) || \
>        (defined (__thumb__) && !defined (__thumb2__)))
> -       "optpld r1\n\t"
> +#ifdef _ISA_ARM_7
> +       "pld    r1\n\t"
> +#endif
>         "eor    r2, r0, r1\n\t"
>         "mov    ip, r0\n\t"
>         "tst    r2, #3\n\t"
> @@ -75,7 +77,9 @@ strcpy (char* dst, const char* src)
>           load stalls.  */
>         ".p2align 2\n"
>    "2:\n\t"
> -       "optpld r1, #8\n\t"
> +#ifdef _ISA_ARM_7
> +       "pld    r1, #8\n\t"
> +#endif
>         "ldr    r4, [r1], #4\n\t"
>         "sub    r2, r3, "magic1(r5)"\n\t"
>         "bics   r2, r2, r3\n\t"
> diff --git a/newlib/libc/machine/arm/strlen-stub.c b/newlib/libc/machine/arm/strlen-stub.c
> index ea45a3789..69cfa3df0 100644
> --- a/newlib/libc/machine/arm/strlen-stub.c
> +++ b/newlib/libc/machine/arm/strlen-stub.c
> @@ -58,7 +58,9 @@ strlen (const char* str)
>         "data .req r3\n\t"
>         "addr .req r1\n\t"
>
> -       "optpld r0\n\t"
> +#ifdef _ISA_ARM_7
> +       "pld r0\n\t"
> +#endif
>         /* Word-align address */
>         "bic    addr, r0, #3\n\t"
>         /* Get adjustment for start ... */
> @@ -113,8 +115,8 @@ strlen (const char* str)
>         "ldreq  data, [addr], #4\n\t"
>         /* and 4 more bytes  */
>         "addeq  len, len, #4\n\t"
> -       /* If we have PLD, then unroll the loop a bit.  */
> -       "optpld addr, #8\n\t"
> +       /* Unroll the loop a bit.  */
> +       "pld    addr, #8\n\t"
>         /*  test (data - 0x01010101)  */
>         "ittt   eq\n\t"
>         "subeq  r2, data, ip\n\t"
> --
> 2.11.0
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]