ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

Error expanding __ASM during compilation #996

Closed mbartling closed 3 years ago

mbartling commented 4 years ago

Hello,

I've been working on including CMSIS-NN/DSP in a CMake project, and so far I've had pretty good luck with Cortex A configurations. However, when I set ARM_CPU to cortex-m* I get the following issue with the core library:

[ 89%] Building C object src/arm/extern/cmsis5/CMSIS/NN/Source/NNSupportFunctions/CMakeFiles/CMSISNNSupport.dir/arm_nn_mat_mult_nt_t_s8.c.o
cd /Users/michael/git/tf/my_project/build/src/arm/extern/cmsis5/CMSIS/NN/Source/NNSupportFunctions && /Users/michael/gcc/gcc-arm-none-eabi-9-2019-q4-major/bin/arm-none-eabi-gcc -DARMCM7_DP -DARM_PROJECT=1 -DCORTEXM -I/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include -I/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/NN/Include -I/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/DSP/Include  -g -ffunction-sections -fdata-sections -mcpu=cortex-m7 -g   -mfloat-abi=hard -mlittle-endian -mthumb -mfpu=fpv5-d16 -o CMakeFiles/CMSISNNSupport.dir/arm_nn_mat_mult_nt_t_s8.c.o   -c /Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/NN/Source/NNSupportFunctions/arm_nn_mat_mult_nt_t_s8.c
In file included from /Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include/cmsis_compiler.h:54,
                 from /Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/DSP/Include/arm_math_types.h:76,
                 from /Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/DSP/Include/arm_math.h:199,
                 from /Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/NN/Source/NNSupportFunctions/arm_nn_mat_mult_nt_t_s8.c:31:
/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include/cmsis_gcc.h: In function 'arm_nn_mat_mult_nt_t_s8':
/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include/cmsis_gcc.h:41:50: warning: asm operand 2 probably doesn't match constraints
   41 |   #define __ASM                                  __asm
      |                                                  ^~~~~
/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include/cmsis_gcc.h:1969:3: note: in expansion of macro '__ASM'
 1969 |   __ASM ("sxtb16 %0, %1, ROR %2" : "=r" (result) : "r" (op1), "i" (rotate) );

 ...
About 599 of these
 ...

 /Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include/cmsis_gcc.h:1969:3: note: in expansion of macro '__ASM'
 1969 |   __ASM ("sxtb16 %0, %1, ROR %2" : "=r" (result) : "r" (op1), "i" (rotate) );
      |   ^~~~~
/Users/michael/git/tf/my_project/extern/CMSIS_5/CMSIS/Core/Include/cmsis_gcc.h:41:50: error: impossible constraint in 'asm'
   41 |   #define __ASM                                  __asm

This only occurs when compiling on my Macbook, but does not occur when compiling in an arm64v8/ubuntu Docker container, even with the same GCC version:

└─ $ ▶ arm-none-eabi-gcc --version
arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 9-2019-q4-major) 9.2.1 20191025 (release) [ARM/arm-9-branch revision 277599]
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Have you guys seen an issue like this before? Thanks!

JonatanAntoni commented 4 years ago

Hi @mbartling,

thanks for reporting this. It looks like GCC does not properly recognize that the second parameter to __SXTB16_RORn is a constant. Hence it reports the warning because a non-constant value cannot be used as an intermediate value (i.e. "i"(rotate)).

We need to check how the GCC implementation of __SXTB16_RORn can be fixed. Do you have any ideas?

Cheers, Jonatan

mbartling commented 4 years ago

Honestly, I've only got a basic working knowledge of the GNU asm extensions. I'll do some deeper digging to see if there is anything I can do to help.

Thanks! -Michael

JonatanAntoni commented 4 years ago

Hello @mbartling,

some GCC compiler experts advised to change the implementation to

#define  __SXTB16_RORn(op1, rotate) \
({ \
  uint32_t result; \
  __ASM ("sxtb16 %0, %1, ROR %2" : "=r" (result) : "r" (op1), "i" (rotate) ); \
  result; \
})

Could you please check if this solves then issue for you?

Cheers, Jonatan

avieira-arm commented 4 years ago

Hi,

I suspect GCC is having trouble 'inlining' the rotate variable. It needs to be an immediate, so the expression must be compile-time known at the call-site. What Jonatan suggested should make that easier if __SXTB16_RORn is used with an immediate argument for 'rotate'. It might also help to beef up the optimization level.

Cheers, Andre

JonatanAntoni commented 4 years ago

Hi @mbartling,

another slightly more elegant implementation might be:

__attribute__((always_inline)) static inline uint32_t  __SXTB16_RORn(uint32_t op1, uint32_t rotate)
{ 
  uint32_t result; 
  if (__builtin_constant_p (rotate) && ((rotate == 8U) || (rotate == 16U) || (rotate == 24U))) {
    asm volatile ("sxtb16 %0, %1, ROR %2" : "=r" (result) : "r" (op1), "i" (rotate) ); 
  } else {
    result = __SXTB16(__ROR(op1, rotate)) ;
  }
  return result;
}

This should automatically boil down into a single "sxtb16 with intermediate ror" instruction if possible. Otherwise it combines plain sxtb with separate ror instruction.

Let me know if one of these solutions work for you and which one is the preferred one.

Cheers, Jonatan

mbartling commented 4 years ago

Hey @JonatanAntoni @avieira-arm, both solutions seem to work and my code compiles CMSIS-NN/DSP to a static library successfully with a lightly modified toolchain file!

Thanks for all your help!

JonatanAntoni commented 4 years ago

Please check the fix in 3551ea8 and close this issue if resolved. Thanks.

mbartling commented 3 years ago

Looks good, sorry got buried in my notifications