[edk2] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S

Message ID 20181220191653.8671-1-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • [edk2] ArmPkg/ArmLib ARM: set .fpu to let Clang 7 assemble ArmV7Support.S
Related show

Commit Message

Ard Biesheuvel Dec. 20, 2018, 7:16 p.m.
Clang 7 complains about the vmsr instruction in ArmV7Support.S,
which is only available on cores that implement some flavour of
VFP. So set the .fpu to NEON like we do in some other places.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +
 1 file changed, 1 insertion(+)

-- 
2.19.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Dec. 21, 2018, 5:58 p.m. | #1
On Thu, Dec 20, 2018 at 08:16:53PM +0100, Ard Biesheuvel wrote:
> Clang 7 complains about the vmsr instruction in ArmV7Support.S,

> which is only available on cores that implement some flavour of

> VFP. So set the .fpu to NEON like we do in some other places.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> index 281499b46cbc..1808962ee3e2 100644

> --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> @@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)

>  #ifndef __clang__

>    mcr     p10,#0x7,r0,c8,c0,#0

>  #else

> +  .fpu    neon

>    vmsr    fpexc, r0

>  #endif


No objection from me. But I would point out that the special clang
filtering here could possibly be dropped.

I mean, theoretically someone could have an even older binutils, but
Linaro GCC 4.8-2013.05 will happily assemble the clang side of this
conditional.

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Dec. 21, 2018, 6 p.m. | #2
On Fri, 21 Dec 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>

> On Thu, Dec 20, 2018 at 08:16:53PM +0100, Ard Biesheuvel wrote:

> > Clang 7 complains about the vmsr instruction in ArmV7Support.S,

> > which is only available on cores that implement some flavour of

> > VFP. So set the .fpu to NEON like we do in some other places.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +

> >  1 file changed, 1 insertion(+)

> >

> > diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> > index 281499b46cbc..1808962ee3e2 100644

> > --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> > +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> > @@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)

> >  #ifndef __clang__

> >    mcr     p10,#0x7,r0,c8,c0,#0

> >  #else

> > +  .fpu    neon

> >    vmsr    fpexc, r0

> >  #endif

>

> No objection from me. But I would point out that the special clang

> filtering here could possibly be dropped.

>

> I mean, theoretically someone could have an even older binutils, but

> Linaro GCC 4.8-2013.05 will happily assemble the clang side of this

> conditional.

>


Yeah, but that would be a separate patch, no?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Dec. 21, 2018, 7:50 p.m. | #3
On Fri, Dec 21, 2018 at 07:00:04PM +0100, Ard Biesheuvel wrote:
> On Fri, 21 Dec 2018 at 18:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> >

> > On Thu, Dec 20, 2018 at 08:16:53PM +0100, Ard Biesheuvel wrote:

> > > Clang 7 complains about the vmsr instruction in ArmV7Support.S,

> > > which is only available on cores that implement some flavour of

> > > VFP. So set the .fpu to NEON like we do in some other places.

> > >

> > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > > ---

> > >  ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 1 +

> > >  1 file changed, 1 insertion(+)

> > >

> > > diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> > > index 281499b46cbc..1808962ee3e2 100644

> > > --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> > > +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S

> > > @@ -268,6 +268,7 @@ ASM_FUNC(ArmEnableVFP)

> > >  #ifndef __clang__

> > >    mcr     p10,#0x7,r0,c8,c0,#0

> > >  #else

> > > +  .fpu    neon

> > >    vmsr    fpexc, r0

> > >  #endif

> >

> > No objection from me. But I would point out that the special clang

> > filtering here could possibly be dropped.

> >

> > I mean, theoretically someone could have an even older binutils, but

> > Linaro GCC 4.8-2013.05 will happily assemble the clang side of this

> > conditional.

> 

> Yeah, but that would be a separate patch, no?


Oh, sure. Just, it would be nice if that could precede this change.

That said, the other two .fpu neon instances in edk2 are both hidden
behind #if !defined(__APPLE__). Should we check with Andrew
(post-wilderbeest) if this is something we need to keep (and if so add
it here) or whether we can delete it across the board?

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
index 281499b46cbc..1808962ee3e2 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
@@ -268,6 +268,7 @@  ASM_FUNC(ArmEnableVFP)
 #ifndef __clang__
   mcr     p10,#0x7,r0,c8,c0,#0
 #else
+  .fpu    neon
   vmsr    fpexc, r0
 #endif
   bx      lr