Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

On Wed, 19 Aug 2009 10:47:14 -0400 (EDT)
Nicolas Pitre <nico@cam.org> wrote:

> > Anyway, I spoke to the GCC people at gcc-help, and they insist that the
> > inlined assembly is wrong. The problem is that GCC doesn't infer from
> > the instruction that it accesses memory, and it can thereby move it out
> > of the loop.
> 
> Oh!  OK.  That's different though.  In your initial report you said that 
> the inline asm was _removed_.  I can understand why it might be factored 
> out of the loop though, but it cannot be removed entirely.

Sorry, I'll be careful about wording in the future :-)

> > -		asm ("ldrd\t%0, [%1]" : "=r" (x) : "r" (io_base));
> > +		asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base) : "memory");
> 
> I think that the early clobber buys you nothing, and the memory clobber 
> is way overkill here.  The volatile ought to be all that is needed.  Can 
> you confirm that only the addition of volatile makes the code OK?  My 
> gcc version is 4.3.2 and that makes no difference what so ever as the 
> code as is produces the correct result already in my case.

Yes, it works fine with 4.3.3 and 4.4.1 with this change. So the
updated patch can be found below. I belive the early clobber should be
there though, since (from the ARM architecture reference manual):

  If <addressing_mode> performs base register write-back and the base
  register <Rn> is one of the two destination registers of the
  instruction, the results are UNPREDICTABLE.

it works fine without the early clobber as well, but I'd feel more safe
having it in.

// Simon
--
Orion NAND: Make asm volatile avoid GCC pushing ldrd out of the loop

GCC 4.3.3 and 4.4.1 happily moves the dword load instruction out of the
loop in orion_nand_read_buf. This patch makes the instruction volatile
to avoid the issue. I've discussed this at gcc-help, refer to the thread
at

  http://gcc.gnu.org/ml/gcc-help/2009-08/msg00187.html

The early clobber is added to avoid the destination registers and the
source register overlapping.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/nand/orion_nand.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Thu, 20 Aug 2009, Simon Kagstrom wrote:

> On Wed, 19 Aug 2009 10:47:14 -0400 (EDT)
> Nicolas Pitre <nico@cam.org> wrote:
> 
> > I think that the early clobber buys you nothing, and the memory clobber 
> > is way overkill here.  The volatile ought to be all that is needed.  Can 
> > you confirm that only the addition of volatile makes the code OK?  My 
> > gcc version is 4.3.2 and that makes no difference what so ever as the 
> > code as is produces the correct result already in my case.
> 
> Yes, it works fine with 4.3.3 and 4.4.1 with this change. So the
> updated patch can be found below. I belive the early clobber should be
> there though, since (from the ARM architecture reference manual):
> 
>   If <addressing_mode> performs base register write-back and the base
>   register <Rn> is one of the two destination registers of the
>   instruction, the results are UNPREDICTABLE.
> 
> it works fine without the early clobber as well, but I'd feel more safe
> having it in.

But this isn't the case here.  We don't perform any writeback.
You get a writeback when you have an addressing mode of the form:

	insn	rd, [rn, rm]!
	insn	rd, [rn, #off]!
	insn	rd, [rn], #off

but not with:

	insn	rd, [rn, #off]

Etc.

Still the early clobber shouldn't have any adverse effect either, unlike 
the memory clobber.

Acked-by: Nicolas Pitre <nico@marvell.com>

Unless the MTD guys are going to pick this patch and push it to Linus 
soon I'll carry it in the Orion git repo.


> 
> // Simon
> --
> Orion NAND: Make asm volatile avoid GCC pushing ldrd out of the loop
> 
> GCC 4.3.3 and 4.4.1 happily moves the dword load instruction out of the
> loop in orion_nand_read_buf. This patch makes the instruction volatile
> to avoid the issue. I've discussed this at gcc-help, refer to the thread
> at
> 
>   http://gcc.gnu.org/ml/gcc-help/2009-08/msg00187.html
> 
> The early clobber is added to avoid the destination registers and the
> source register overlapping.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/mtd/nand/orion_nand.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c
> index 7ad9722..0d9d4bc 100644
> --- a/drivers/mtd/nand/orion_nand.c
> +++ b/drivers/mtd/nand/orion_nand.c
> @@ -61,7 +61,7 @@ static void orion_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>  	buf64 = (uint64_t *)buf;
>  	while (i < len/8) {
>  		uint64_t x;
> -		asm ("ldrd\t%0, [%1]" : "=r" (x) : "r" (io_base));
> +		asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>  		buf64[i++] = x;
>  	}
>  	i *= 8;
> -- 
> 1.6.0.4
>

On Thu, 20 Aug 2009 12:14:47 -0400 (EDT)
Nicolas Pitre <nico@cam.org> wrote:

> > Yes, it works fine with 4.3.3 and 4.4.1 with this change. So the
> > updated patch can be found below. I belive the early clobber should be
> > there though, since (from the ARM architecture reference manual):
> > 
> >   If <addressing_mode> performs base register write-back and the base
> >   register <Rn> is one of the two destination registers of the
> >   instruction, the results are UNPREDICTABLE.
> > 
> > it works fine without the early clobber as well, but I'd feel more safe
> > having it in.
> 
> But this isn't the case here.  We don't perform any writeback.
> You get a writeback when you have an addressing mode of the form:
> 
> 	insn	rd, [rn, rm]!
> 	insn	rd, [rn, #off]!
> 	insn	rd, [rn], #off
> 
> but not with:
> 
> 	insn	rd, [rn, #off]

OK, I'm still a beginner on the ARM architecture, so thanks for the
explanation!

> Still the early clobber shouldn't have any adverse effect either, unlike 
> the memory clobber.
> 
> Acked-by: Nicolas Pitre <nico@marvell.com>
> 
> Unless the MTD guys are going to pick this patch and push it to Linus 
> soon I'll carry it in the Orion git repo.

Great, thanks!

// Simon