On Mon, Jan 11, 2021, at 7:21 AM, Richard Earnshaw wrote:
> Some initial comments.
>
> On 11/01/2021 11:10, g...@danielengel.com wrote:
> > From: Daniel Engel <g...@danielengel.com>
> >
> > These definitions facilitate subsequent patches in this series.
> >
> > gcc/libgcc/ChangeLog:
> > 2021-01-07 Daniel Engel <g...@danielengel.com>
> >
> > * config/arm/t-elf: Organize functions into logical groups.
> > * config/arm/lib1funcs.S: Add FUNC_START macro variations for
> > weak functions and manual control of the target section;
> > rename THUMB_FUNC_START as THUMB_FUNC_ENTRY for consistency;
> > removed unused macros THUMB_SYNTAX, ARM_SYM_START, SYM_END;
> > removed redundant syntax directives.
>
> This needs to be re-formatted using the correct ChangeLog style, which
> is in most cases
>
> * <filename> (<function-or-macro-name>): <Summary of change>.
>
> You can repeat for multiple functions in the same file, but leave off
> the "* <filename>" part as long as they are contiguous in the log.
Will do. Sorry.
> > ---
> > libgcc/config/arm/lib1funcs.S | 114 +++++++++++++++++++---------------
> > libgcc/config/arm/t-elf | 55 +++++++++++++---
> > 2 files changed, 110 insertions(+), 59 deletions(-)
> >
> > diff --git a/libgcc/config/arm/lib1funcs.S b/libgcc/config/arm/lib1funcs.S
> > index c2fcfc503ec..b4541bae791 100644
> > --- a/libgcc/config/arm/lib1funcs.S
> > +++ b/libgcc/config/arm/lib1funcs.S
> > @@ -69,11 +69,13 @@ see the files COPYING3 and COPYING.RUNTIME
> > respectively. If not, see
> > #define TYPE(x) .type SYM(x),function
> > #define SIZE(x) .size SYM(x), . - SYM(x)
> > #define LSYM(x) .x
> > +#define LLSYM(x) .L##x
> > #else
> > #define __PLT__
> > #define TYPE(x)
> > #define SIZE(x)
> > #define LSYM(x) x
> > +#define LLSYM(x) x
> > #endif
>
> I can live with this.
>
> >
> > /* Function end macros. Variants for interworking. */
> > @@ -247,6 +249,14 @@ LSYM(Lend_fde):
> >
> > #define COND(op1, op2, cond) op1 ## op2 ## cond
> >
> > +#ifdef __ARM_FEATURE_IT
> > + #define IT(ins,c) ins##c
> > +#else
> > + // Assume default Thumb-1 flags-affecting suffix 's'.
> > + // Almost all instructions require this in unified syntax.
> > + #define IT(ins,c) ins##s
>
> This simply doesn't make much sense, at least, not enough to make it
> generally available. It seems it would be invariably wrong to replace a
> conditional instruction in arm/thumb2 code with a non-conditional flag
> setting instruction in thumb1. So please don't do this as it's likely
> to be a source of bugs going forwards if folk don't understand exactly
> when it is safe.
I 'm going to push back to use this approach. I'm a huge believer in
DRY code, and duplicating sequences of 1-4 instructions in a dozen
different places feels unclean. Duplicating instructions also makes
code comments cumbersome, particularly when doing something tricky in
the conditional block.
I do understand your concern about the __ARM_FEATURE_IT name stepping
into ARM's namespace. I chose that as a low-mental-friction pattern and
I'd like to keep any replacement similar. Is __HAVE_FEATURE_IT OK?
This macro currently saves ~10 #ifdef blocks, and I expect that the
number to rise significantly if/when I have time to merge ieee754-sf.S
(One example might be __mulsf3() since it's relatively independent. It's
now 360 bytes in v7m, and 96 bytes on v6m. I would just need to go
through the new version and a few Thumb-2 optimizations, such as using
the hardware multiply instructions instead of __mulsidi3.)
The safety you want boils down to whether or not __HAVE_FEATURE_IT gets
set correctly. The do_it() macro seems universally used within libgcc
to support both Thumb-2 and arm compilation of the same code. I've
defined __HAVE_FEATURE_IT to have the same scope as do_it(), and the
assembler checks that conditionals are consistent with the previous IT.
While using the IT() macro without do_it() could result in unintended "s"
suffix instructions being emitted for Thumb-1, compilation will fail
when attempting to build any Thumb-2 multilib. At that point, adding
do_it() macro will lead to __HAVE_FEATURE_IT and everything should
be self-evident.
I want the macro name to be short, so that it fits within one indent.
I briefly considered _(), but figured that would be too obtuse.
I will add the following comment before the macro to clarify:
/* The IT(c) macro streamlines the construction of short branchless
conditional sequences that support ARM, Thumb-2, and Thumb-1.
It is meant as an extension to the .do_it macro defined above.
Code not written to support Thumb-1 need not use IT(c).
Where the IT instruction is supported by ARM and Thumb-2, the
given instruction compiles with the conditional suffix 'c'.
Since Thumb-1 and v6m do not support IT, the given instruction
compiles with the standard unified syntax suffix "s". This is
somewhat simplistic and does not support 'cmp', or 'tst' since
they do not have the suffix "s". It also fails for 'movs' and
'adds' with high registers as operands. Such uses will result
in a compiler error. However, it seems unlikely that code
written with Thumb-1 support in mind will need such patterns,
so the simplistic solution will serve for now.
Typical if/then/else usage is:
#ifdef __ARM_FEATURE_IT
// ARM and Thumb-2 'true' condition.
do_it c, t
#else
// Thumb-1 'false' condition. This must be opposite the
// sense of the ARM and Thumb-2 condition, since the
// branch is taken to skip the 'true' instruction block.
b!c else_label
#endif
// Conditional 'true' execution for all compile modes.
IT(ins1,c) op1, op2
IT(ins2,c) op1, op2
#ifndef __ARM_FEATURE_IT
// Thumb-1 branch to skip the 'else' instruction block.
// Omitted for if/then usage.
b end_label,
#endif
else_label:
// Conditional 'false' execution for all compile modes.
// Omitted for if/then usage.
IT(ins3,!c) op1, op2
IT(ins4,!c) op1, op2
end_label:
// Unconditional execution resumes here.
*/
None of this matters unless someone is specifically writing a Thumb-1
compatible function that also provides Thumb-2 performance. That is the
real complexity. It's my opinion that minimizing the number of
divergence points is the best way to prevent future bugs. If I had any
reasonable hope of teaching do_it() about "opposite" logic conditions, I
would have written a macro to absorb the initial #ifdef too.
(As a related aside, I also find the RETc(xx) preprocessor macro
distracting, and would like to see it replaced with a .macro directive
so that the columns can line up without parenthesis).
>
> > +#endif
> > +
> > #ifdef __ARM_EABI__
> > .macro ARM_LDIV0 name signed
> > cmp r0, #0
> > @@ -280,7 +290,6 @@ LSYM(Lend_fde):
> > pop {r1, pc}
> >
> > #elif defined(__thumb2__)
> > - .syntax unified
>
> OK - I think this is now set unconditionally at the top level.
>
> > .ifc \signed, unsigned
> > cbz r0, 1f
> > mov r0, #0xffffffff
> > @@ -324,10 +333,6 @@ LSYM(Lend_fde):
> > .endm
> > #endif
> >
> > -.macro FUNC_END name
> > - SIZE (__\name)
> > -.endm
> > -
>
> Moved later, OK.
>
> > .macro DIV_FUNC_END name signed
> > cfi_start __\name, LSYM(Lend_div0)
> > LSYM(Ldiv0):
> > @@ -340,48 +345,64 @@ LSYM(Ldiv0):
> > FUNC_END \name
> > .endm
> >
> > -.macro THUMB_FUNC_START name
> > - .globl SYM (\name)
> > - TYPE (\name)
> > - .thumb_func
> > -SYM (\name):
> > -.endm
>
> I'm not really sure what you reasoning is for removing the *FUNC_START
> macros and then adding almost identical macros of the form *FUNC_ENTRY.
> It seems to me like unnecessary churn.
I did not generally remove the *FUNC_START macros. Both FUNC_START and
ARM_FUNC_START still exist. If you follow the cumulative macro expansion, you
will find they still generate exactly the same directives.
However, the existing FUNC_START macro assumes that it will always be
used at the start of a monolithic single-entry-point function. In this
usage, the ".text" directive that those macros insert is OK.
In the new code, there are multiple instances where a second entry point
occurs in the middle of a function. As easy example is __paritydi2()
also providing __paritysi2(). In such cases I want to export a global
symbol, but I don't want a ".text" from FUNC_START to reset the
assembler state. (This was functionality provided by the CM0_ macros
you objected to the original version of the library).
So, FUNC_ENTRY is simply FUNC_START minus ".text". The way this
library was architected, it's all really necessary. Try to compile
bits/parity.S with ENTRY changed to START, and you will get errors from
both CFI and SIZE() calculations. The name of course is arbitrary, but
I think it fits in fairly well with the traditional names. If you want
to make higher impact changes and refactor the rest of libgcc, an
alternate pattern might be START_SECTION_FUNC, START_TEXT_FUNC, and
MID_SECTION_FUNC.
With the names as they are now, it seemed cleaner to redefine FUNC_START
as FUNC_ENTRY plus ".text". I can duplicate the directives if you would
rather not touch FUNC_START.
Finally, since THUMB_FUNC_START never had the ".text" directive, it
seemed consistent to rename it as THUMB_FUNC_ENTRY. I observed
that every usage of THUMB_FUNC_ENTRY also had a .force_thumb
directive, so I folded that into the macro definition.
Perhaps I need to add this explanation to the commit message.
> > -
> > /* Function start macros. Variants for ARM and Thumb. */
> > -
> > #ifdef __thumb__
> > #define THUMB_FUNC .thumb_func
> > #define THUMB_CODE .force_thumb
> > -# if defined(__thumb2__)
> > -#define THUMB_SYNTAX
> > -# else
> > -#define THUMB_SYNTAX
> > -# endif
> > #else
> > #define THUMB_FUNC
> > #define THUMB_CODE
> > -#define THUMB_SYNTAX
>
> OK. Dead code.
>
> > #endif
> >
> > -.macro FUNC_START name
> > - .text
> > +.macro THUMB_FUNC_ENTRY name
> > + .globl SYM (\name)
> > + TYPE (\name)
> > + .force_thumb
> > + .thumb_func
> > +SYM (\name):
> > +.endm
> > +
> > +/* Strong global export, no section change. */
> > +.macro FUNC_ENTRY name
> > .globl SYM (__\name)
> > TYPE (__\name)
> > - .align 0
>
> It's often wrong to assume the section is correctly aligned on entry -
> so removing this is suspect, unless you're adding explicit alignment
> before every function instance. I know the issue of litterals in the
> code segments is currently still under discussion; but if they do exist,
> they have to be 32-bit aligned because the thumb1 adr instruction will
> not work correctly otherwise. Arm code must always be (at least) 32-bit
> aligned, and thumb code 16-bit - but there are often significant
> performance wins from being more aligned than that on entry.
Discussed above. The ENTRY directives will never be at the start of a
section. It is only intended to be used within a function body. I can
expand the comments to make this clearer.
>
>
> > THUMB_CODE
> > THUMB_FUNC
> > - THUMB_SYNTAX
> > SYM (__\name):
> > .endm
> >
> > -.macro ARM_SYM_START name
> > - TYPE (\name)
> > - .align 0
> > -SYM (\name):
> > +/* Weak global export, no section change. */
> > +.macro WEAK_ENTRY name
> > + .weak SYM(__\name)
> > + FUNC_ENTRY \name
> > +.endm
> > +
> > +/* Strong global export, explicit section. */
> > +.macro FUNC_START_SECTION name section
> > + .section \section,"x"
> > + .align 0
> > + FUNC_ENTRY \name
> > .endm
> >
> > -.macro SYM_END name
> > - SIZE (\name)
> > +/* Weak global export, explicit section. */
> > +.macro WEAK_START_SECTION name section
> > + .weak SYM(__\name)
> > + FUNC_START_SECTION \name \section
> > +.endm
> > +
> > +/* Strong global export, default section. */
> > +.macro FUNC_START name
> > + FUNC_START_SECTION \name .text
> > +.endm
> > +
> > +/* Weak global export, default section. */
> > +.macro WEAK_START name
> > + .weak SYM(__\name)
> > + FUNC_START_SECTION \name .text
> > +.endm
> > +
> > +.macro FUNC_END name
> > + SIZE (__\name)
> > .endm
> >
> > /* Special function that will always be coded in ARM assembly, even if
> > @@ -392,7 +413,6 @@ SYM (\name):
> > /* For Thumb-2 we build everything in thumb mode. */
> > .macro ARM_FUNC_START name
> > FUNC_START \name
> > - .syntax unified
> > .endm
> > #define EQUIV .thumb_set
> > .macro ARM_CALL name
> > @@ -447,6 +467,11 @@ SYM (__\name):
> > #endif
> > .endm
> >
> > +.macro WEAK_ALIAS new old
> > + .weak SYM(__\new)
> > + FUNC_ALIAS \new \old
> > +.endm
> > +
> > #ifndef NOT_ISA_TARGET_32BIT
> > .macro ARM_FUNC_ALIAS new old
> > .globl SYM (__\new)
> > @@ -1905,10 +1930,9 @@ ARM_FUNC_START ctzsi2
> >
> > .text
> > .align 0
> > - .force_thumb
> >
> > .macro call_via register
> > - THUMB_FUNC_START _call_via_\register
> > + THUMB_FUNC_ENTRY _call_via_\register>
> > bx \register
> > nop
> > @@ -1991,7 +2015,7 @@ _arm_return_r11:
> > .macro interwork_with_frame frame, register, name, return
> > .code 16
> >
> > - THUMB_FUNC_START \name
> > + THUMB_FUNC_ENTRY \name
> >
>
> This, and all the subsequent instances of this change would go away if
> you kept the original name.
Discussed above. I don't use the THUMB_FUNC_START anywhere in the new
code. However, since this isn't the start of a new ".text" section,
it seemed worthwhile to make the naming convention consistent
throughout libgcc.
> > bx pc
> > nop
> > @@ -2008,7 +2032,7 @@ _arm_return_r11:
> > .macro interwork register
> > .code 16
> >
> > - THUMB_FUNC_START _interwork_call_via_\register
> > + THUMB_FUNC_ENTRY _interwork_call_via_\register
> >
> > bx pc
> > nop
> > @@ -2045,7 +2069,7 @@ LSYM(Lchange_\register):
> > /* The LR case has to be handled a little differently... */
> > .code 16
> >
> > - THUMB_FUNC_START _interwork_call_via_lr
> > + THUMB_FUNC_ENTRY _interwork_call_via_lr
> >
> > bx pc
> > nop
> > @@ -2073,9 +2097,7 @@ LSYM(Lchange_\register):
> >
> > .text
> > .align 0
> > - .force_thumb
>
> I think .force_thumb is required to enable this code to build when the
> base architecture is ARMv4 or earlier. It's a bit of a wart, but needs
> careful testing if it is to be removed.
Not removed. Folded into the macro definition. The usage of
.force_thumb and .thumb_func is now tightly coupled throughout
libgcc.
>
> > - .syntax unified
> > - THUMB_FUNC_START __gnu_thumb1_case_sqi
> > + THUMB_FUNC_ENTRY __gnu_thumb1_case_sqi
> > push {r1}
> > mov r1, lr
> > lsrs r1, r1, #1
> > @@ -2092,9 +2114,7 @@ LSYM(Lchange_\register):
> >
> > .text
> > .align 0
> > - .force_thumb
> > - .syntax unified
> > - THUMB_FUNC_START __gnu_thumb1_case_uqi
> > + THUMB_FUNC_ENTRY __gnu_thumb1_case_uqi
> > push {r1}
> > mov r1, lr
> > lsrs r1, r1, #1
> > @@ -2111,9 +2131,7 @@ LSYM(Lchange_\register):
> >
> > .text
> > .align 0
> > - .force_thumb
> > - .syntax unified
> > - THUMB_FUNC_START __gnu_thumb1_case_shi
> > + THUMB_FUNC_ENTRY __gnu_thumb1_case_shi
> > push {r0, r1}
> > mov r1, lr
> > lsrs r1, r1, #1
> > @@ -2131,9 +2149,7 @@ LSYM(Lchange_\register):
> >
> > .text
> > .align 0
> > - .force_thumb
> > - .syntax unified
> > - THUMB_FUNC_START __gnu_thumb1_case_uhi
> > + THUMB_FUNC_ENTRY __gnu_thumb1_case_uhi
> > push {r0, r1}
> > mov r1, lr
> > lsrs r1, r1, #1
> > @@ -2151,9 +2167,7 @@ LSYM(Lchange_\register):
> >
> > .text
> > .align 0
> > - .force_thumb
> > - .syntax unified
> > - THUMB_FUNC_START __gnu_thumb1_case_si
> > + THUMB_FUNC_ENTRY __gnu_thumb1_case_si
> > push {r0, r1}
> > mov r1, lr
> > adds.n r1, r1, #2 /* Align to word. */
> > diff --git a/libgcc/config/arm/t-elf b/libgcc/config/arm/t-elf
> > index 9da6cd37054..6a31a328073 100644
> > --- a/libgcc/config/arm/t-elf
> > +++ b/libgcc/config/arm/t-elf
> > @@ -18,15 +18,52 @@ endif # !__symbian__
> > # However this is not true for ARMv6M. Here we want to use the soft-fp C
> > # implementation. The soft-fp code is only build for ARMv6M. This pulls
> > # in the asm implementation for other CPUs.
> > -LIB1ASMFUNCS += _udivsi3 _divsi3 _umodsi3 _modsi3 _dvmd_tls _bb_init_func \
> > - _call_via_rX _interwork_call_via_rX \
> > - _lshrdi3 _ashrdi3 _ashldi3 \
> > - _arm_negdf2 _arm_addsubdf3 _arm_muldivdf3 _arm_cmpdf2 _arm_unorddf2 \
> > - _arm_fixdfsi _arm_fixunsdfsi \
> > - _arm_truncdfsf2 _arm_negsf2 _arm_addsubsf3 _arm_muldivsf3 \
> > - _arm_cmpsf2 _arm_unordsf2 _arm_fixsfsi _arm_fixunssfsi \
> > - _arm_floatdidf _arm_floatdisf _arm_floatundidf _arm_floatundisf \
> > - _clzsi2 _clzdi2 _ctzsi2
> > +
> > +# Group 1: Integer functions
> > +LIB1ASMFUNCS += \
> > + _ashldi3 \
> > + _ashrdi3 \
> > + _lshrdi3 \
> > + _clzdi2 \
> > + _clzsi2 \
> > + _ctzsi2 \
> > + _dvmd_tls \
> > + _divsi3 \
> > + _modsi3 \
> > + _udivsi3 \
> > + _umodsi3 \
> > +
> > +# Group 2: Single precision floating point
> > +LIB1ASMFUNCS += \
> > + _arm_addsubsf3 \
> > + _arm_cmpsf2 \
> > + _arm_fixsfsi \
> > + _arm_fixunssfsi \
> > + _arm_floatdisf \
> > + _arm_floatundisf \
> > + _arm_muldivsf3 \
> > + _arm_negsf2 \
> > + _arm_unordsf2 \
> > +
> > +# Group 3: Double precision floating point
> > +LIB1ASMFUNCS += \
> > + _arm_addsubdf3 \
> > + _arm_cmpdf2 \
> > + _arm_fixdfsi \
> > + _arm_fixunsdfsi \
> > + _arm_floatdidf \
> > + _arm_floatundidf \
> > + _arm_muldivdf3 \
> > + _arm_negdf2 \
> > + _arm_truncdfsf2 \
> > + _arm_unorddf2 \
> > +
> > +# Group 4: Miscellaneous
> > +LIB1ASMFUNCS += \
> > + _bb_init_func \
> > + _call_via_rX \
> > + _interwork_call_via_rX \
> > +
>
> This bit is OK.
>
> >
> > # Currently there is a bug somewhere in GCC's alias analysis
> > # or scheduling code that is breaking _fpmul_parts in fp-bit.c.
> >
>
> R.
>
