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

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

[Simon Kagstrom]

GCC 4.3.3 happily removes the dword load instruction in
orion_nand_read_buf. This patch makes the inline assembly volatile to
avoid this situation.

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..3f9b113 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

Re: [PATCH] Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

[Nicolas Pitre]

On Tue, 14 Jul 2009, Simon Kagstrom wrote:

> GCC 4.3.3 happily removes the dword load instruction in
> orion_nand_read_buf. This patch makes the inline assembly volatile to
> avoid this situation.

If GCC does optimize away this inline asm then this is a serious GCC 
bug.  The inline asm uses an output operand for which a dependency 
exists on the very next line.

If I were you I'd try to find a non broken GCC version instead.


> 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..3f9b113 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
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

Re: [PATCH] Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

[Simon Kagstrom]

On Tue, 14 Jul 2009 12:40:33 -0400 (EDT)
Nicolas Pitre <nico@cam.org> wrote:

> On Tue, 14 Jul 2009, Simon Kagstrom wrote:
> 
> > GCC 4.3.3 happily removes the dword load instruction in
> > orion_nand_read_buf. This patch makes the inline assembly volatile to
> > avoid this situation.
> 
> If GCC does optimize away this inline asm then this is a serious GCC 
> bug.  The inline asm uses an output operand for which a dependency 
> exists on the very next line.

I've been away on vacation, sorry for not replying until now.

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.

So here comes an updated patch.

// Simon
--
Orion NAND: Clobber memory to make sure that GCC does not move ldrd

GCC 4.3.3 happily removes the dword load instruction in
orion_nand_read_buf. This patch clobbers memory, and 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

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..845f9a9 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) : "memory");
 		buf64[i++] = x;
 	}
 	i *= 8;
-- 
1.6.0.4

Re: [PATCH] Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

[Nicolas Pitre]

On Wed, 19 Aug 2009, Simon Kagstrom wrote:

> On Tue, 14 Jul 2009 12:40:33 -0400 (EDT)
> Nicolas Pitre <nico@cam.org> wrote:
> 
> > On Tue, 14 Jul 2009, Simon Kagstrom wrote:
> > 
> > > GCC 4.3.3 happily removes the dword load instruction in
> > > orion_nand_read_buf. This patch makes the inline assembly volatile to
> > > avoid this situation.
> > 
> > If GCC does optimize away this inline asm then this is a serious GCC 
> > bug.  The inline asm uses an output operand for which a dependency 
> > exists on the very next line.
> 
> I've been away on vacation, sorry for not replying until now.
> 
> 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.

> So here comes an updated patch.
> 
> // Simon
> --
> Orion NAND: Clobber memory to make sure that GCC does not move ldrd
> 
> GCC 4.3.3 happily removes the dword load instruction in
> orion_nand_read_buf. This patch clobbers memory, and 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
> 
> 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..845f9a9 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) : "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.


Nicolas

Re: [PATCH] Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

[Simon Kagstrom]

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(-)

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

Re: [PATCH] Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

[Nicolas Pitre]

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
> 

Re: [PATCH] Orion NAND: Make dword load asm volatile to avoid GCC optimizing it away

[Simon Kagstrom]

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