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
