diff mbox series

[U-Boot,07/30] riscv: set -march and -mabi based on the Kconfig configuration

Message ID 20181019220743.15020-8-lukas.auer@aisec.fraunhofer.de
State Superseded
Delegated to: Andes
Headers show
Series General fixes / cleanup for RISC-V and improvements to qemu-riscv | expand

Commit Message

Lukas Auer Oct. 19, 2018, 10:07 p.m. UTC 
Use the new Kconfig entries to construct the ISA string for the -march
compiler flag. The -mabi compiler flag is selected based on the base
integer instruction set.

With this change, the C (compressed instructions) ISA extension is now
enabled for all boards with CONFIG_RISCV_ISA_C set. Buildman reports a
decrease in binary size of 71590 bytes.

Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
---

 arch/riscv/Makefile  | 13 +++++++++++++
 arch/riscv/config.mk |  4 ----
 2 files changed, 13 insertions(+), 4 deletions(-)

Comments

Bin Meng Oct. 22, 2018, 7:21 a.m. UTC | #1 
Hi Lukas,

On Sat, Oct 20, 2018 at 6:09 AM Lukas Auer
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Use the new Kconfig entries to construct the ISA string for the -march
> compiler flag. The -mabi compiler flag is selected based on the base
> integer instruction set.
>
> With this change, the C (compressed instructions) ISA extension is now
> enabled for all boards with CONFIG_RISCV_ISA_C set. Buildman reports a
> decrease in binary size of 71590 bytes.
>
> Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> ---
>
>  arch/riscv/Makefile  | 13 +++++++++++++
>  arch/riscv/config.mk |  4 ----
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 8fb6a889d8..6fb292d0b4 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -3,6 +3,19 @@
>  # Copyright (C) 2017 Andes Technology Corporation.
>  # Rick Chen, Andes Technology Corporation <rick@andestech.com>
>
> +riscv-march-$(CONFIG_ARCH_RV32I)       := rv32im
> +riscv-march-$(CONFIG_ARCH_RV64I)       := rv64im
> +riscv-march-$(CONFIG_RISCV_ISA_A)      := $(riscv-march-y)a
> +riscv-march-$(CONFIG_RISCV_ISA_C)      := $(riscv-march-y)c
> +
> +riscv-mabi-$(CONFIG_ARCH_RV64I)        := lp64
> +riscv-mabi-$(CONFIG_ARCH_RV32I)        := ilp32
> +
> +arch-y := -march=$(riscv-march-y) -mabi=$(riscv-mabi-y)
> +
> +PLATFORM_CPPFLAGS      += $(arch-y)
> +CFLAGS_EFI             += $(arch-y)
> +

The concept of this patch is good. However the usage of := is a bit
odd, since it makes people think the latter will override the former
one, however it is not.

Can we get rid of these riscv-mach-xxx, instead using something like this:

ifeq ($(CONFIG_RISCV_ISA_A),y)
        ARCH_A = a
endif
ifeq ($(CONFIG_RISCV_ISA_C),y)
        ARCH_C = c
endif

ifeq ($(CONFIG_ARCH_RV32I),y)
        BITS = 32
        ABI_I = i
endif
ifeq ($(CONFIG_ARCH_RV64I),y)
        BITS = 64
endif

PLATFORM_CPPFLAGS += -march=rv$(BITS)im$(ARCH_A)$(ARCH_C)
-mabi=$(ABI_I)lp$(BITS)

>  head-y := arch/riscv/cpu/start.o
>
>  libs-y += arch/riscv/cpu/
> diff --git a/arch/riscv/config.mk b/arch/riscv/config.mk
> index ed9eb0c24c..9088b9ef2c 100644
> --- a/arch/riscv/config.mk
> +++ b/arch/riscv/config.mk
> @@ -14,16 +14,12 @@
>  64bit-emul             := elf64lriscv
>
>  ifdef CONFIG_32BIT
> -PLATFORM_CPPFLAGS      += -march=rv32ima -mabi=ilp32
>  PLATFORM_LDFLAGS       += -m $(32bit-emul)
> -CFLAGS_EFI             += -march=rv32ima -mabi=ilp32
>  EFI_LDS                        := elf_riscv32_efi.lds
>  endif
>
>  ifdef CONFIG_64BIT
> -PLATFORM_CPPFLAGS      += -march=rv64ima -mabi=lp64
>  PLATFORM_LDFLAGS       += -m $(64bit-emul)
> -CFLAGS_EFI             += -march=rv64ima -mabi=lp64
>  EFI_LDS                        := elf_riscv64_efi.lds
>  endif
>
> --

Regards,
Bin
Lukas Auer Oct. 24, 2018, 3:57 p.m. UTC | #2 
Hi Bin,

On Mon, 2018-10-22 at 15:21 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Sat, Oct 20, 2018 at 6:09 AM Lukas Auer
> <lukas.auer@aisec.fraunhofer.de> wrote:
> > 
> > Use the new Kconfig entries to construct the ISA string for the
> > -march
> > compiler flag. The -mabi compiler flag is selected based on the
> > base
> > integer instruction set.
> > 
> > With this change, the C (compressed instructions) ISA extension is
> > now
> > enabled for all boards with CONFIG_RISCV_ISA_C set. Buildman
> > reports a
> > decrease in binary size of 71590 bytes.
> > 
> > Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> > ---
> > 
> >  arch/riscv/Makefile  | 13 +++++++++++++
> >  arch/riscv/config.mk |  4 ----
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 8fb6a889d8..6fb292d0b4 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -3,6 +3,19 @@
> >  # Copyright (C) 2017 Andes Technology Corporation.
> >  # Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > 
> > +riscv-march-$(CONFIG_ARCH_RV32I)       := rv32im
> > +riscv-march-$(CONFIG_ARCH_RV64I)       := rv64im
> > +riscv-march-$(CONFIG_RISCV_ISA_A)      := $(riscv-march-y)a
> > +riscv-march-$(CONFIG_RISCV_ISA_C)      := $(riscv-march-y)c
> > +
> > +riscv-mabi-$(CONFIG_ARCH_RV64I)        := lp64
> > +riscv-mabi-$(CONFIG_ARCH_RV32I)        := ilp32
> > +
> > +arch-y := -march=$(riscv-march-y) -mabi=$(riscv-mabi-y)
> > +
> > +PLATFORM_CPPFLAGS      += $(arch-y)
> > +CFLAGS_EFI             += $(arch-y)
> > +
> 
> The concept of this patch is good. However the usage of := is a bit
> odd, since it makes people think the latter will override the former
> one, however it is not.
> 
> Can we get rid of these riscv-mach-xxx, instead using something like
> this:
> 
> ifeq ($(CONFIG_RISCV_ISA_A),y)
>         ARCH_A = a
> endif
> ifeq ($(CONFIG_RISCV_ISA_C),y)
>         ARCH_C = c
> endif
> 
> ifeq ($(CONFIG_ARCH_RV32I),y)
>         BITS = 32
>         ABI_I = i
> endif
> ifeq ($(CONFIG_ARCH_RV64I),y)
>         BITS = 64
> endif
> 
> PLATFORM_CPPFLAGS += -march=rv$(BITS)im$(ARCH_A)$(ARCH_C)
> -mabi=$(ABI_I)lp$(BITS)
> 

That makes sense. Yes I can change it to something like that. 
One small change I would do is to try and keep any constant ISA / ABI
strings (so rv and im) out of PLATFORM_CPPFLAGS to keep the
configuration more in one place. So something like this. What do you
think?

ifeq ($(CONFIG_ARCH_RV32I),y)
        ARCH_BASE = rv32im
        ABI = ilp32
endif
ifeq ($(CONFIG_ARCH_RV64I),y)
        ARCH_BASE = rv64im
        ABI = lp64
endif

PLATFORM_CPPFLAGS += -march=$(ARCH_BASE)$(ARCH_A)$(ARCH_C)
-mabi=$(ABI)

Thanks,
Lukas

> >  head-y := arch/riscv/cpu/start.o
> > 
> >  libs-y += arch/riscv/cpu/
> > diff --git a/arch/riscv/config.mk b/arch/riscv/config.mk
> > index ed9eb0c24c..9088b9ef2c 100644
> > --- a/arch/riscv/config.mk
> > +++ b/arch/riscv/config.mk
> > @@ -14,16 +14,12 @@
> >  64bit-emul             := elf64lriscv
> > 
> >  ifdef CONFIG_32BIT
> > -PLATFORM_CPPFLAGS      += -march=rv32ima -mabi=ilp32
> >  PLATFORM_LDFLAGS       += -m $(32bit-emul)
> > -CFLAGS_EFI             += -march=rv32ima -mabi=ilp32
> >  EFI_LDS                        := elf_riscv32_efi.lds
> >  endif
> > 
> >  ifdef CONFIG_64BIT
> > -PLATFORM_CPPFLAGS      += -march=rv64ima -mabi=lp64
> >  PLATFORM_LDFLAGS       += -m $(64bit-emul)
> > -CFLAGS_EFI             += -march=rv64ima -mabi=lp64
> >  EFI_LDS                        := elf_riscv64_efi.lds
> >  endif
> > 
> > --
> 
> Regards,
> Bin
Bin Meng Oct. 25, 2018, 1:56 a.m. UTC | #3 
Hi Lukas,

On Wed, Oct 24, 2018 at 11:57 PM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Mon, 2018-10-22 at 15:21 +0800, Bin Meng wrote:
> > Hi Lukas,
> >
> > On Sat, Oct 20, 2018 at 6:09 AM Lukas Auer
> > <lukas.auer@aisec.fraunhofer.de> wrote:
> > >
> > > Use the new Kconfig entries to construct the ISA string for the
> > > -march
> > > compiler flag. The -mabi compiler flag is selected based on the
> > > base
> > > integer instruction set.
> > >
> > > With this change, the C (compressed instructions) ISA extension is
> > > now
> > > enabled for all boards with CONFIG_RISCV_ISA_C set. Buildman
> > > reports a
> > > decrease in binary size of 71590 bytes.
> > >
> > > Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> > > ---
> > >
> > >  arch/riscv/Makefile  | 13 +++++++++++++
> > >  arch/riscv/config.mk |  4 ----
> > >  2 files changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > index 8fb6a889d8..6fb292d0b4 100644
> > > --- a/arch/riscv/Makefile
> > > +++ b/arch/riscv/Makefile
> > > @@ -3,6 +3,19 @@
> > >  # Copyright (C) 2017 Andes Technology Corporation.
> > >  # Rick Chen, Andes Technology Corporation <rick@andestech.com>
> > >
> > > +riscv-march-$(CONFIG_ARCH_RV32I)       := rv32im
> > > +riscv-march-$(CONFIG_ARCH_RV64I)       := rv64im
> > > +riscv-march-$(CONFIG_RISCV_ISA_A)      := $(riscv-march-y)a
> > > +riscv-march-$(CONFIG_RISCV_ISA_C)      := $(riscv-march-y)c
> > > +
> > > +riscv-mabi-$(CONFIG_ARCH_RV64I)        := lp64
> > > +riscv-mabi-$(CONFIG_ARCH_RV32I)        := ilp32
> > > +
> > > +arch-y := -march=$(riscv-march-y) -mabi=$(riscv-mabi-y)
> > > +
> > > +PLATFORM_CPPFLAGS      += $(arch-y)
> > > +CFLAGS_EFI             += $(arch-y)
> > > +
> >
> > The concept of this patch is good. However the usage of := is a bit
> > odd, since it makes people think the latter will override the former
> > one, however it is not.
> >
> > Can we get rid of these riscv-mach-xxx, instead using something like
> > this:
> >
> > ifeq ($(CONFIG_RISCV_ISA_A),y)
> >         ARCH_A = a
> > endif
> > ifeq ($(CONFIG_RISCV_ISA_C),y)
> >         ARCH_C = c
> > endif
> >
> > ifeq ($(CONFIG_ARCH_RV32I),y)
> >         BITS = 32
> >         ABI_I = i
> > endif
> > ifeq ($(CONFIG_ARCH_RV64I),y)
> >         BITS = 64
> > endif
> >
> > PLATFORM_CPPFLAGS += -march=rv$(BITS)im$(ARCH_A)$(ARCH_C)
> > -mabi=$(ABI_I)lp$(BITS)
> >
>
> That makes sense. Yes I can change it to something like that.
> One small change I would do is to try and keep any constant ISA / ABI
> strings (so rv and im) out of PLATFORM_CPPFLAGS to keep the
> configuration more in one place. So something like this. What do you
> think?
>

This looks good. Thanks!

> ifeq ($(CONFIG_ARCH_RV32I),y)
>         ARCH_BASE = rv32im
>         ABI = ilp32
> endif
> ifeq ($(CONFIG_ARCH_RV64I),y)
>         ARCH_BASE = rv64im
>         ABI = lp64
> endif
>
> PLATFORM_CPPFLAGS += -march=$(ARCH_BASE)$(ARCH_A)$(ARCH_C)
> -mabi=$(ABI)
>

Regards,
Bin
diff mbox series

Patch

diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 8fb6a889d8..6fb292d0b4 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -3,6 +3,19 @@ 
 # Copyright (C) 2017 Andes Technology Corporation.
 # Rick Chen, Andes Technology Corporation <rick@andestech.com>
 
+riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32im
+riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64im
+riscv-march-$(CONFIG_RISCV_ISA_A)	:= $(riscv-march-y)a
+riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
+
+riscv-mabi-$(CONFIG_ARCH_RV64I)	:= lp64
+riscv-mabi-$(CONFIG_ARCH_RV32I)	:= ilp32
+
+arch-y := -march=$(riscv-march-y) -mabi=$(riscv-mabi-y)
+
+PLATFORM_CPPFLAGS	+= $(arch-y)
+CFLAGS_EFI		+= $(arch-y)
+
 head-y := arch/riscv/cpu/start.o
 
 libs-y += arch/riscv/cpu/
diff --git a/arch/riscv/config.mk b/arch/riscv/config.mk
index ed9eb0c24c..9088b9ef2c 100644
--- a/arch/riscv/config.mk
+++ b/arch/riscv/config.mk
@@ -14,16 +14,12 @@ 
 64bit-emul		:= elf64lriscv
 
 ifdef CONFIG_32BIT
-PLATFORM_CPPFLAGS	+= -march=rv32ima -mabi=ilp32
 PLATFORM_LDFLAGS	+= -m $(32bit-emul)
-CFLAGS_EFI		+= -march=rv32ima -mabi=ilp32
 EFI_LDS			:= elf_riscv32_efi.lds
 endif
 
 ifdef CONFIG_64BIT
-PLATFORM_CPPFLAGS	+= -march=rv64ima -mabi=lp64
 PLATFORM_LDFLAGS	+= -m $(64bit-emul)
-CFLAGS_EFI		+= -march=rv64ima -mabi=lp64
 EFI_LDS			:= elf_riscv64_efi.lds
 endif