[ARM,GAS] Allow integer immediate for VFP vmov instructions.

Message ID 20180510110633.GA5507@arm.com
State New
Headers show
Series
  • [ARM,GAS] Allow integer immediate for VFP vmov instructions.
Related show

Commit Message

Tamar Christina May 10, 2018, 11:10 a.m.
Hi All,

This patch fixes the case where you want to use an integer value the
floating point immediate to a VFP vmov instruction such as
vmovmi.f32 s27, #11.

If the immediate is not a float we convert it and copy it's representation
into the imm field and then carry on validating as if we originally entered
a floating point immediate.

The is considered a QoL improvement for hand assembly writers and allows more
code portability between assembler.

Cross compiled and regtested:

 * arm-none-eabi
 * armeb-none-eabi

and no regressions.

Bootstrapped and regtested on:

 * arm-none-eabi
 * arm-none-eabi (64 bit host)

and no regressions.

Ok for master?

Thanks,
Tamar

gas/
2018-05-10  Tamar Christina  <tamar.christina@arm.com>
	* gas/config/tc-arm.c (do_neon_mov): Allow integer literal for float
	immediate.
	* testsuite/gas/arm/vfp-mov-enc.s: New.
	* testsuite/gas/arm/vfp-mov-enc.d: New.

--

Comments

Nick Clifton May 10, 2018, 11:16 a.m. | #1
Hi Tamar,

> If the immediate is not a float we convert it and copy it's representation

> into the imm field and then carry on validating as if we originally entered

> a floating point immediate.


I think that you ought to verify that the conversion to floating point 
representation has not lost any accuracy, and issue a warning/error if
the floating point and immediate values are not the same.

Cheers
  Nick
Tamar Christina May 10, 2018, 2:46 p.m. | #2
Hi Nick,

The 05/10/2018 12:16, Nick Clifton wrote:
> Hi Tamar,

> 

> > If the immediate is not a float we convert it and copy it's representation

> > into the imm field and then carry on validating as if we originally entered

> > a floating point immediate.

> 

> I think that you ought to verify that the conversion to floating point 

> representation has not lost any accuracy, and issue a warning/error if

> the floating point and immediate values are not the same.


If there's a significant loss of precision you're going to get an error already due to
is_quarter_float rejecting the float then as it wouldn't be representable.

You also can't have a loss due to a large value, as if the value is large enough not to fit
inside a float it will also not fit in the imm encoding for the instruction, so you'll get
an error saying "garbage after ...".

I have a patch locally that adds a warning using truncf (imm) != imm but have so far been
unable to trigger any failing case.  I can submit it if you'd like to have a warning here
but it also adds a dependency to libm.  I assume that's not a problem?


I assume the AArch64 variant of this patch (submitted at the same time) wouldn't need this
as that just has a more liberal parsing and I don't know whether it was an integer at all.


Regards,
Tamar

> 

> Cheers

>   Nick

> 


--
Nick Clifton May 10, 2018, 2:55 p.m. | #3
Hi Tamar,

>> I think that you ought to verify that the conversion to floating point 

>> representation has not lost any accuracy, and issue a warning/error if

>> the floating point and immediate values are not the same.

> 

> If there's a significant loss of precision you're going to get an error already due to

> is_quarter_float rejecting the float then as it wouldn't be representable.

> 

> You also can't have a loss due to a large value, as if the value is large enough not to fit

> inside a float it will also not fit in the imm encoding for the instruction, so you'll get

> an error saying "garbage after ...".

> 

> I have a patch locally that adds a warning using truncf (imm) != imm but have so far been

> unable to trigger any failing case.  I can submit it if you'd like to have a warning here

> but it also adds a dependency to libm.  I assume that's not a problem?


That's not a problem, although I was assuming that you would just cast the float
result back to an int and then compare that with the original integer value.

But, if as you say, it is not possible to get to this point with an invalid integer
constant, then please disregard my comment.

So - patch approved - please apply.


> I assume the AArch64 variant of this patch (submitted at the same time) wouldn't need this

> as that just has a more liberal parsing and I don't know whether it was an integer at all.


Correct - that patch is OK too.

Cheers
  Nick

Patch

diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
index eda989092169ac8c977bbb36f4d151a85bf6ea60..f11be8cf53a65e1fc3b170efcb39a9072cc4fd0e 100644
--- a/gas/config/tc-arm.c
+++ b/gas/config/tc-arm.c
@@ -16705,6 +16705,14 @@  do_neon_mov (void)
     case NS_FI:  /* case 10 (fconsts).  */
       ldconst = "fconsts";
       encode_fconstd:
+      if (!inst.operands[1].immisfloat)
+	{
+	  /* Immediate has to fit in 8 bits so float is enough.  */
+	  float imm = (float)inst.operands[1].imm;
+	  memcpy (&inst.operands[1].imm, &imm, sizeof (float));
+	  inst.operands[1].immisfloat = 1;
+	}
+
       if (is_quarter_float (inst.operands[1].imm))
 	{
 	  inst.operands[1].imm = neon_qfloat_bits (inst.operands[1].imm);
diff --git a/gas/testsuite/gas/arm/vfp-mov-enc.d b/gas/testsuite/gas/arm/vfp-mov-enc.d
new file mode 100644
index 0000000000000000000000000000000000000000..5c4b266e8aa2bebbe91316da65bd7c9b6162e35a
--- /dev/null
+++ b/gas/testsuite/gas/arm/vfp-mov-enc.d
@@ -0,0 +1,13 @@ 
+# name: VFP check vmov supports integer immediates
+# as: -mcpu=cortex-a8 -mfpu=vfpv3
+# objdump: -dr --prefix-addresses --show-raw-insn
+
+.*: +file format .*arm.*
+
+Disassembly of section .text:
+0[0-9a-f]+ <[^>]+> 4ef2da06 	vmovmi.f32	s27, #38	; 0x41300000  11.0
+0[0-9a-f]+ <[^>]+> 4ef2da06 	vmovmi.f32	s27, #38	; 0x41300000  11.0
+0[0-9a-f]+ <[^>]+> 4ef7da00 	vmovmi.f32	s27, #112	; 0x3f800000  1.0
+0[0-9a-f]+ <[^>]+> 4ef7da00 	vmovmi.f32	s27, #112	; 0x3f800000  1.0
+0[0-9a-f]+ <[^>]+> cebb1b04 	vmovgt.f64	d1, #180	; 0xc1a00000 -20.0
+0[0-9a-f]+ <[^>]+> ceb81b00 	vmovgt.f64	d1, #128	; 0xc0000000 -2.0
diff --git a/gas/testsuite/gas/arm/vfp-mov-enc.s b/gas/testsuite/gas/arm/vfp-mov-enc.s
new file mode 100644
index 0000000000000000000000000000000000000000..4362fb1d399f9495d443e6a0ebd1bf1e775e1dee
--- /dev/null
+++ b/gas/testsuite/gas/arm/vfp-mov-enc.s
@@ -0,0 +1,6 @@ 
+VMOVMI.F32 s27,#11
+VMOVMI.F32 s27,#11.0
+VMOVMI.F32 s27,#1
+VMOVMI.F32 s27,#1.0
+VMOVGT.F64 d1,#-20
+VMOVGT.F64 d1,#-2