* Re: [PATCH] MTD: Fix Orion NAND driver compilation with ARM OABI
2010-03-20 11:58 ` David Woodhouse
@ 2010-03-20 14:41 ` Nicolas Pitre
2010-03-20 15:06 ` Mikael Pettersson
2010-03-21 17:16 ` Jamie Lokier
2 siblings, 0 replies; 8+ messages in thread
From: Nicolas Pitre @ 2010-03-20 14:41 UTC (permalink / raw)
To: David Woodhouse
Cc: Paulius Zaleckas, linux-kernel, linux-mtd, u.kleine-koenig,
simon.kagstrom, akpm, linux-arm-kernel, rth
On Sat, 20 Mar 2010, David Woodhouse wrote:
> On Sat, 2010-03-20 at 13:20 +0200, Paulius Zaleckas wrote:
> > On Sat, Mar 20, 2010 at 11:41 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> > > On Sat, 2010-03-20 at 10:55 +0200, Paulius Zaleckas wrote:
> > >> - uint64_t x;
> > >> + /*
> > >> + * force x variable to r2/r3 registers since ldrd instruction
> > >> + * requires first register to be even.
> > >> + */
> > >> + register uint64_t x asm ("r2");
> > >> +
> > >> asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
> > >
> > > Hm, isn't there an asm constraint which will force it into an
> > > appropriate register pair?
> >
> > Not that I know of...
Digging into the gcc code there is indeed no way to specify an even
register pair as a constraint. What's there is:
#define TARGET_LDRD (arm_arch5e && ARM_DOUBLEWORD_ALIGN)
and
#define ARM_DOUBLEWORD_ALIGN TARGET_AAPCS_BASED
where AAPCS is equivalent to EABI.
> > > Failing that, "=&r2,r4,r6,r8" ought to work.
> >
> > No, fails with error: matching constraint not valid in output operand
>
> Hm, crap -- GCC on ARM doesn't let you give specific registers, so that
> trick doesn't work.
>
> Strictly speaking, I think your version is wrong -- although you force
> the variable 'x' to be stored in r2/r3, you don't actually force GCC to
> use r2/r3 as the output registers for the asm statement -- it could
> happily use other registers, then move their contents into r2/r3
> afterwards.
Actually that's the other way around, and the GCC manual is
contradicting you. The value can be moved in other registers, but at
the point of reference (which is the inline asm in this case) then the
value has to be in the specified register. Quoting the GCC manual in
section 5.40 "Variables in Specified Registers":
|Local register variables in specific registers do not reserve the
|registers, except at the point where they are used as input or output
|operands in an `asm' statement and the `asm' statement itself is not
|deleted. The compiler's data flow analysis is capable of determining
|where the specified registers contain live values, and where they are
|available for other uses. Stores into local register variables may be
|deleted when they appear to be dead according to dataflow analysis.
|References to local register variables may be deleted or moved or
|simplified.
|
|These local variables are sometimes convenient for use with
|the extended `asm' feature (*note Extended Asm::), if you want to write
|one output of the assembler instruction directly into a particular
|register. (This will work provided the register you specify fits the
|constraints specified for that operand in the `asm'.)
And that last paragraph is precisely what is being relied
upon here. Otherwise what would be the purpose left for this construct?
> Obviously it _won't_ do that most of the time, but it _could_. GCC PR
> #15089 was filed for the fact that sometimes it does, but I think Nico
> was missing the point -- GCC is _allowed_ to do that, and if it makes
> you sad then you should be asking for better assembly constraints which
> would allow you to tell it not to.
No. PR #15089 was filed because this has been broken at some point when
some code generation optimizations kicked in. And the bug as now been
fixed for more than 5 years now.
> See the __asmeq() macro in <asm/system.h> for a dirty hack which will
> check which registers are used and abort at compile time, although your
> compilation is going to fail anyway so I'm not sure it makes much of a
> difference in this particular case.
The __asmeq hack has been used as a safety guard so not to ban what used
to be the current GCC version at the time this bug was discovered.
Since this bug seemed to appear much less frequently
when using -Os instead of -O2 (the default on ARM) then the __asmeq was
a good way to still use the broken GCC without ending up with broken
code unknowingly.
Unless people are still using ancient GCC versions, there is little
reason to worry about this bug anymore.
Just to say that I don't think that Paulius' version is wrong. Maybe
suboptimal but not wrong. And his choice of r2 matches with the
compiler's choice in the EABI case anyway, so in practice the code is as
optimal as it would otherwise be.
> The real fix here is to add an asm constraint to GCC which allows you to
> specify "any even GPR" (or whatever's most suitable for the ldrd
> instruction). Being able to give specific registers, like you can on
> other architectures, would be useful too.
I agree completely with the fact that a proper constraint is lacking.
Being able to give a specific register is already supported though,
although this is not directly through the inline asm constraint.
> Please file a PR, then resubmit your patch with a comment explaining
> that the code is known to be broken because GCC doesn't allow you to do
> any better, and containing a reference to your PR. If people copy your
> code, I want them to at least know that they're propagating a bug.
This is sensible I guess.
Nicolas
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] MTD: Fix Orion NAND driver compilation with ARM OABI
2010-03-20 11:58 ` David Woodhouse
2010-03-20 14:41 ` Nicolas Pitre
@ 2010-03-20 15:06 ` Mikael Pettersson
2010-03-20 15:37 ` Nicolas Pitre
2010-03-21 17:16 ` Jamie Lokier
2 siblings, 1 reply; 8+ messages in thread
From: Mikael Pettersson @ 2010-03-20 15:06 UTC (permalink / raw)
To: David Woodhouse
Cc: Paulius Zaleckas, nico, linux-kernel, linux-mtd, u.kleine-koenig,
simon.kagstrom, akpm, nico, linux-arm-kernel, rth
David Woodhouse writes:
> On Sat, 2010-03-20 at 13:20 +0200, Paulius Zaleckas wrote:
> > On Sat, Mar 20, 2010 at 11:41 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> > > On Sat, 2010-03-20 at 10:55 +0200, Paulius Zaleckas wrote:
> > >> - uint64_t x;
> > >> + /*
> > >> + * force x variable to r2/r3 registers since ldrd instruction
> > >> + * requires first register to be even.
> > >> + */
> > >> + register uint64_t x asm ("r2");
> > >> +
> > >> asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
> > >
> > > Hm, isn't there an asm constraint which will force it into an
> > > appropriate register pair?
> >
> > Not that I know of...
> >
> > > Failing that, "=&r2,r4,r6,r8" ought to work.
> >
> > No, fails with error: matching constraint not valid in output operand
>
> Hm, crap -- GCC on ARM doesn't let you give specific registers, so that
> trick doesn't work.
I missed the start of this thread, but looking at orion_nand.c I fail to
see why you'd need to mess with inline asm for reading a sequence of u64
values from an I/O location to an array.
Rewriting that to proper C generates a nice ldrd;strd loop with my gcc-4.4.3.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] MTD: Fix Orion NAND driver compilation with ARM OABI
2010-03-20 15:06 ` Mikael Pettersson
@ 2010-03-20 15:37 ` Nicolas Pitre
0 siblings, 0 replies; 8+ messages in thread
From: Nicolas Pitre @ 2010-03-20 15:37 UTC (permalink / raw)
To: Mikael Pettersson
Cc: Paulius Zaleckas, lkml, linux-mtd, u.kleine-koenig,
simon.kagstrom, Andrew Morton, David Woodhouse, linux-arm-kernel,
rth
On Sat, 20 Mar 2010, Mikael Pettersson wrote:
> I missed the start of this thread, but looking at orion_nand.c I fail to
> see why you'd need to mess with inline asm for reading a sequence of u64
> values from an I/O location to an array.
The reason for doing so with inline asm in the first place was not
discussed in this thread.
> Rewriting that to proper C generates a nice ldrd;strd loop with my gcc-4.4.3.
Sure, GCC version 4.4.3 does the right thing. Not all of them do.
Nicolas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MTD: Fix Orion NAND driver compilation with ARM OABI
2010-03-20 11:58 ` David Woodhouse
2010-03-20 14:41 ` Nicolas Pitre
2010-03-20 15:06 ` Mikael Pettersson
@ 2010-03-21 17:16 ` Jamie Lokier
2 siblings, 0 replies; 8+ messages in thread
From: Jamie Lokier @ 2010-03-21 17:16 UTC (permalink / raw)
To: David Woodhouse
Cc: Paulius Zaleckas, nico, linux-kernel, linux-mtd, u.kleine-koenig,
simon.kagstrom, akpm, nico, linux-arm-kernel, rth
David Woodhouse wrote:
> Strictly speaking, I think your version is wrong -- although you force
> the variable 'x' to be stored in r2/r3, you don't actually force GCC to
> use r2/r3 as the output registers for the asm statement -- it could
> happily use other registers, then move their contents into r2/r3
> afterwards.
We used to do that a lot in the syscall macros in <asm/unistd.h>, on a
lot of architectures. Were they all broken?
> Obviously it _won't_ do that most of the time, but it _could_. GCC PR
> #15089 was filed for the fact that sometimes it does, but I think Nico
> was missing the point -- GCC is _allowed_ to do that, and if it makes
> you sad then you should be asking for better assembly constraints which
> would allow you to tell it not to.
>From the GCC info documentation:
Sometimes you need to make an `asm' operand be a specific register,
but there's no matching constraint letter for that register _by
itself_. To force the operand into that register, use a local variable
for the operand and specify the register in the variable declaration.
*Note Explicit Reg Vars::. Then for the `asm' operand, use any
register constraint letter that matches the register:
register int *p1 asm ("r0") = ...;
register int *p2 asm ("r1") = ...;
register int *result asm ("r0");
asm ("sysint" : "=r" (result) : "0" (p1), "r" (p2));
Fwiw, that note is present in GCC-4.0.1, but not GCC-3.3.6. But we've
depended on that behaviour for a long time.
Note that we've depended on GCC not copying values with a dereferenced
memory location for a long time too: E.g. "+m" (*ptr) is used a lot in
spinlocks.
I'm sure I've read email confirmation (on these very lists) that GCC
will always work with a memory constraint used in that way, without
copying the value to/from a different location such as a stack slot.
But suprisingly, the GCC documentation says:
Extended asm supports input-output or read-write operands. Use
the constraint character `+' to indicate such an operand and list
it with the output operands. You should only use read-write
operands when the constraints for the operand (or the operand in
which only some of the bits are to be changed) allow a register.
^^^^^^^^^^^^^^^^
Maybe we're relying on undefined GCC behaviour for the "+m" constraint?
> See the __asmeq() macro in <asm/system.h> for a dirty hack which will
> check which registers are used and abort at compile time, although your
> compilation is going to fail anyway so I'm not sure it makes much of a
> difference in this particular case.
>
> The real fix here is to add an asm constraint to GCC which allows you to
> specify "any even GPR" (or whatever's most suitable for the ldrd
> instruction). Being able to give specific registers, like you can on
> other architectures, would be useful too.
See above GCC documentation for using register variables to designate
specific registers. Many supported architectures don't have asm
letter constraints for each register - hence so many of the old
_syscallN macros in <asm/unistd.h> having to use register variables.
I am surprised GCC doesn't have a constraint for "any even register
suitable for ldrd" on ARM, but I've just checked gcc-4.4.3 and it doesn't.
However, if I'm reading the source correctly, if not compiling for
Thumb-1, and GCC believes the target machine supports ldrd, then all
doubleword values are constrained to an even register pair anyway.
That's why GCC itself does not need an even-register constraint letter.
...Which is I guess why it throws up only with OABI, or with pre-arm5e
archs: GCC doesn't consider OABI targets to support ldrd. (It's
actually some more obscure condition, let's not go there).
Something else from the lovely GCC source:
mfix-cortex-m3-ldrd
Target Report Var(fix_cm3_ldrd) Init(2)
Avoid overlapping destination and address registers on LDRD instructions
that may trigger Cortex-M3 errata.
In other words, the "=&" earlyclobber *is* needed on Cortex-M3.
Enjoy!
-- Jamie
^ permalink raw reply [flat|nested] 8+ messages in thread