Hi Christian,

Thanks for looking at this. I will need to read the code in detail but this is a first top level reivew.

On 09/29/14 12:03, Christian Bruel wrote:
Hi Ramana, Richard,

This patch implements the attribute target (and pragma) to allow
function based interworking.

as in the updated documentation, the syntax is:

  __attribute__((target("thumb"))) int foo()
Forces thumb mode for function foo only. If the file was compiled with
-mthumb iit has no effect.

Indeed



Similarly

  __attribute__((target("arm"))) int foo()
Forces arm mode for function foo. It has no effect if the file was not
compiled with -mthumb.

Indeed.


and regions can be grouped together with

#pragma GCC target ("thumb")
or
#pragma GCC target ("arm")

a few notes
- Inlining is allowed between functions of the same mode (compilation
switch, #pragma and attribute)

Why shouldn't we allow inlining between functions of ARM mode vs Thumb mode ? After all the choice of mode is irrelevant at the time of inlining (except possibly for inline assembler).

Perhaps an option is to try to disable inlining in the presence of inline assembler or if not gate it from a command line option.

- 'arm_option_override' is now reorganized around
'arm_option_override_internal' for thumb related macros

Looks like a reasonable start - We need a couple of tests to make sure that __attribute__(("arm")) on a file compiled for the M profile results in a syntax error. v7(e)m is Thumb2 only.

for bonus points it would be great to get __attribute__(("target")) working properly in the backend. I suspect a number of the tuning flags and the global architecture state needs to be moved into this as well to handle cases where __attribute__(("arm")) used with M profile options is error'd out.

- I kept TARGET_UNIFIED_ASM to minimize changes. Although removing it
would avoid to switch between unified/divided asms

I know Terry's been trying to get Thumb1 to also switch by default to unified asm. So I think a lot of the logic with "emit_thumb" could just go away. Maybe we should just consider switching ARM state to unified syntax and that would be as simple as changing TARGET_UNIFIED_SYNTAX in arm.h to be TARGET_32BIT. Long overdue IMHO.

The only gotcha here is inline assembler but GAS is so permissive that I'm not too worried about it in ARM state and Thumb2 state. I'm a bit worried about Thumb1.


   and simplify arm_declare_function_name. Should be considered at some
point.

I think that can be done for a lot of newer cores - some of that logic is dated now IIUC.

I remember why my original project failed - I couldn't get enough of the backend in shape for the state to be saved and restored and then I moved on to other more interesting things, so whatever is done here needs to make sure that all ISA mode state is saved and restored.

One thing I experimented with while doing this work was adding something like the mflip-mips16 option and then have the testsuite run with this option to try and make sure enough of the backend state is saved and restored properly.

This will give more testing coverage hopefully to the switching logic for the attributes and hopefully expose any issues that are there with respect to saving and restoring state. The problem you'll probably find is that in some of the gcc.target/arm tests the flipping of state will cause various interesting failures. The other side is making sure that all state that is global is now captured and reinitialized everytime we switch context between ARM and Thumb.

I'm not sure how to best test the pragma switching logic but given that the 2 hang off each other, it should just work (TM) if one is right. In any case adding some testcases that direct test this would be useful.


- It is only available for Thumb2 variants (for thumb1 lack of interest
and a few complications I was unable to test, although this could be
added easily if needed, I think)


I don't think we should restrict this to Thumb1 or Thumb2 it should be allowed if the architecture allows it.

For example __attribute__((thumb)) on a function compiled with -march=armv5te -mfloat-abi=softfp -mfpu=vfpv3 should give a syntax error as this is not supported. No VFP instructions in Thumb1. Explicit tests for this would be appreciated.

IIRC all other cases should be accepted.



Tested for no regression for arm-none-eabi [,-with-arch=armv7-a]

   OK for trunk ?

Sorry not yet.

I would also like some documentation added to extend.texi for these attributes.

So in summary.

1. Some documentation in extend.texi please.
2. TARGET_UNIFIED_SYNTAX to be turned on for ARM state too.
3. Testcases for this and some testing with a mflip-thumbness switch (added only for testing).
4. Investigate further giving up restriction on Thumb1.
5. Investigate lifting inlining restriction for __attribute__(("arm")) or __attribute__ (("thumb")) or maybe gate it off a command line option. 6. Split this patch into smaller logical chunks that can help with review i.e. the restructuring from the attribute and pragma addition, the testcases. 7. Tests for the diagnostics and error cases i.e. __attribute__(("arm")) used with -march=armv7em -march=armv7m command line options.


regards
Ramana






Reply via email to