[PATCH] powerpc/dcr: Use cmplwi instead of 3-argument cmpli

[PATCH] powerpc/dcr: Use cmplwi instead of 3-argument cmpli

[Michael Ellerman]

In dcr-low.S we use cmpli with three arguments, instead of four
arguments as defined in the ISA:

	cmpli	cr0,r3,1024

This appears to be a PPC440-ism, looking at the "PPC440x5 CPU Core
User’s Manual" it shows cmpli having no L field, but implied to be 0 due
to the core being 32-bit. It mentions that the ISA defines four
arguments and recommends using cmplwi.

dcr-low.S is only built 32-bit, because it is only built when
DCR_NATIVE=y, which is only selected by 40x and 44x. Looking at the
generated code (with gcc/gas) we see cmplwi as expected.

Although gas is happy with the 3-argument version when building for
32-bit, the LLVM assembler is not and errors out with:

  arch/powerpc/sysdev/dcr-low.S:27:10: error: invalid operand for instruction
   cmpli 0,%r3,1024; ...
           ^

Switching to the four argument version avoids any confusion when reading
the ISA, fixes the issue with the LLVM assembler, and also means the
code could be built 64-bit in future (though that's very unlikely).

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/sysdev/dcr-low.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/dcr-low.S b/arch/powerpc/sysdev/dcr-low.S
index efeeb1b885a1..329b9c4ae542 100644
--- a/arch/powerpc/sysdev/dcr-low.S
+++ b/arch/powerpc/sysdev/dcr-low.S
@@ -11,7 +11,7 @@
 #include <asm/export.h>
 
 #define DCR_ACCESS_PROLOG(table) \
-	cmpli	cr0,r3,1024;	 \
+	cmplwi	cr0,r3,1024;	 \
 	rlwinm  r3,r3,4,18,27;   \
 	lis     r5,table@h;      \
 	ori     r5,r5,table@l;   \
-- 
2.25.1

Re: [PATCH] powerpc/dcr: Use cmplwi instead of 3-argument cmpli

[Segher Boessenkool]

Hi!

On Thu, Oct 14, 2021 at 01:44:24PM +1100, Michael Ellerman wrote:
> In dcr-low.S we use cmpli with three arguments, instead of four
> arguments as defined in the ISA:
> 
> 	cmpli	cr0,r3,1024
> 
> This appears to be a PPC440-ism, looking at the "PPC440x5 CPU Core
> User’s Manual" it shows cmpli having no L field, but implied to be 0 due
> to the core being 32-bit. It mentions that the ISA defines four
> arguments and recommends using cmplwi.

It also corresponds to the old POWER instruction set, which had no L
field there, a reserved bit instead.  It used to be that -many allowed
these insns as well, but not anymore.

> Although gas is happy with the 3-argument version when building for
> 32-bit, the LLVM assembler is not and errors out with:

A GAS targeting powerpc64 isn't happy either, fwiw.

>   arch/powerpc/sysdev/dcr-low.S:27:10: error: invalid operand for instruction
>    cmpli 0,%r3,1024; ...
>            ^
> 
> Switching to the four argument version avoids any confusion when reading
> the ISA, fixes the issue with the LLVM assembler, and also means the
> code could be built 64-bit in future (though that's very unlikely).

You are actually now using to the extended opcode cmpwli (a much better
plan :-) )

Thanks,


Segher

Re: [PATCH] powerpc/dcr: Use cmplwi instead of 3-argument cmpli

[Nick Desaulniers]

On Wed, Oct 13, 2021 at 7:44 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> In dcr-low.S we use cmpli with three arguments, instead of four
> arguments as defined in the ISA:
>
>         cmpli   cr0,r3,1024
>
> This appears to be a PPC440-ism, looking at the "PPC440x5 CPU Core
> User’s Manual" it shows cmpli having no L field, but implied to be 0 due
> to the core being 32-bit. It mentions that the ISA defines four
> arguments and recommends using cmplwi.
>
> dcr-low.S is only built 32-bit, because it is only built when
> DCR_NATIVE=y, which is only selected by 40x and 44x. Looking at the
> generated code (with gcc/gas) we see cmplwi as expected.
>
> Although gas is happy with the 3-argument version when building for
> 32-bit, the LLVM assembler is not and errors out with:
>
>   arch/powerpc/sysdev/dcr-low.S:27:10: error: invalid operand for instruction
>    cmpli 0,%r3,1024; ...
>            ^
>
> Switching to the four argument version avoids any confusion when reading
> the ISA, fixes the issue with the LLVM assembler, and also means the
> code could be built 64-bit in future (though that's very unlikely).

Thank you Michael.  We've definitely run into a few cases where GAS
allowed for various short-hand forms of various instructions (a fair
amount of recent work was around 32b ARM and THUMB parity in LLVM).
LLVM's assembler is mostly generated from a high level description of
the instruction formats, so it's not always as flexible as a hand
written parser would be. (There is a mix of hand written arch specific
parsing, but most of the parser is arch agnostic, and all of the
instruction descriptions are described in an LLVM specific high level
language called tablegen which generates C++ that is used by the
assembler, but also the disassembler, the compiler, and even the
linker if need be).

Link: https://github.com/ClangBuiltLinux/linux/issues/1419
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/sysdev/dcr-low.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/sysdev/dcr-low.S b/arch/powerpc/sysdev/dcr-low.S
> index efeeb1b885a1..329b9c4ae542 100644
> --- a/arch/powerpc/sysdev/dcr-low.S
> +++ b/arch/powerpc/sysdev/dcr-low.S
> @@ -11,7 +11,7 @@
>  #include <asm/export.h>
>
>  #define DCR_ACCESS_PROLOG(table) \
> -       cmpli   cr0,r3,1024;     \
> +       cmplwi  cr0,r3,1024;     \
>         rlwinm  r3,r3,4,18,27;   \
>         lis     r5,table@h;      \
>         ori     r5,r5,table@l;   \
> --
> 2.25.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] powerpc/dcr: Use cmplwi instead of 3-argument cmpli

[Michael Ellerman]

On Thu, 14 Oct 2021 13:44:24 +1100, Michael Ellerman wrote:
> In dcr-low.S we use cmpli with three arguments, instead of four
> arguments as defined in the ISA:
> 
> 	cmpli	cr0,r3,1024
> 
> This appears to be a PPC440-ism, looking at the "PPC440x5 CPU Core
> User’s Manual" it shows cmpli having no L field, but implied to be 0 due
> to the core being 32-bit. It mentions that the ISA defines four
> arguments and recommends using cmplwi.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/dcr: Use cmplwi instead of 3-argument cmpli
      https://git.kernel.org/powerpc/c/fef071be57dc43679a32d5b0e6ee176d6f12e9f2

cheers