Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Kyrill Tkachov]

Hi Thomas,

On 18/04/18 12:31, Thomas Preudhomme wrote:

Hi Kyrill,

On 11/04/18 10:02, Kyrill Tkachov wrote:

Hi Thomas,

On 09/04/18 15:29, Thomas Preudhomme wrote:

Hi Ramana,

On 06/04/18 17:17, Thomas Preudhomme wrote:
>
>
> On 06/04/18 17:08, Ramana Radhakrishnan wrote:
>> On 06/04/2018 16:54, Thomas Preudhomme wrote:
>>> Instruction pattern for setting the FPSCR expects the input value to be
>>> in a register. However, __builtin_arm_set_fpscr expander does not ensure
>>> that this is the case and as a result GCC ICEs when the builtin is
>>> called with a constant literal.
>>>
>>> This commit fixes the builtin to force the input value into a register.
>>> It also remove the unneeded volatile in the existing fpscr test and
>>> fixes the function prototype.
>>>
>>> ChangeLog entries are as follows:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2018-04-06  Thomas Preud'homme 
>>>
>>> PR target/85261
>>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
>>> into register.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2018-04-06  Thomas Preud'homme 
>>>
>>> PR target/85261
>>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
>>> literal value.  Expect 2 MCR instruction. Fix function prototype.
>>> Remove volatile keyword.
>>>
>>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
>>> no regression.
>>>
>>> Is this ok for stage4?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>
>> (sorry about the duplicate for those who get it)
>>
>>
>> LGTM, though in this case I would prefer a bootstrap and regression run
>> as this is automatically exercised most with gcc.dg/atomic_*.c and you
>> really need this tested on linux than just bare-metal as I'm not sure
>> how this gets tested on arm-none-eabi.
>
> Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap
> right away.

Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4
--with-float=hard --enable-languages=c,c++,fortran --with-system-zlib
--enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any
regression either.

Ok to commit?



Thanks for doing this.
This is ok for trunk.


>
>>
>> What about earlier branches, have you looked ? This is a silly target
>> bug and fixes should go back to older branches in this particular case
>> after baking this on trunk for some time.
>
> GCC 6 and 7 are affected as well and a backport will be done once it has baked
> long enough of course.

Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that
is finished.


Backports show no regression on a bootstrapped arm-none-linux-gnueabihf GCC 6 & 
7. Ok to commit those?



Yes, thanks.
Kyrill


Best regards,

Thomas

Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Thomas Preudhomme]

Hi Kyrill,

On 11/04/18 10:02, Kyrill Tkachov wrote:

Hi Thomas,

On 09/04/18 15:29, Thomas Preudhomme wrote:

Hi Ramana,

On 06/04/18 17:17, Thomas Preudhomme wrote:
>
>
> On 06/04/18 17:08, Ramana Radhakrishnan wrote:
>> On 06/04/2018 16:54, Thomas Preudhomme wrote:
>>> Instruction pattern for setting the FPSCR expects the input value to be
>>> in a register. However, __builtin_arm_set_fpscr expander does not ensure
>>> that this is the case and as a result GCC ICEs when the builtin is
>>> called with a constant literal.
>>>
>>> This commit fixes the builtin to force the input value into a register.
>>> It also remove the unneeded volatile in the existing fpscr test and
>>> fixes the function prototype.
>>>
>>> ChangeLog entries are as follows:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2018-04-06  Thomas Preud'homme 
>>>
>>> PR target/85261
>>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
>>> into register.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2018-04-06  Thomas Preud'homme 
>>>
>>> PR target/85261
>>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
>>> literal value.  Expect 2 MCR instruction. Fix function prototype.
>>> Remove volatile keyword.
>>>
>>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
>>> no regression.
>>>
>>> Is this ok for stage4?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>
>> (sorry about the duplicate for those who get it)
>>
>>
>> LGTM, though in this case I would prefer a bootstrap and regression run
>> as this is automatically exercised most with gcc.dg/atomic_*.c and you
>> really need this tested on linux than just bare-metal as I'm not sure
>> how this gets tested on arm-none-eabi.
>
> Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap
> right away.

Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4
--with-float=hard --enable-languages=c,c++,fortran --with-system-zlib
--enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any
regression either.

Ok to commit?



Thanks for doing this.
This is ok for trunk.


>
>>
>> What about earlier branches, have you looked ? This is a silly target
>> bug and fixes should go back to older branches in this particular case
>> after baking this on trunk for some time.
>
> GCC 6 and 7 are affected as well and a backport will be done once it has baked
> long enough of course.

Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that
is finished.


Backports show no regression on a bootstrapped arm-none-linux-gnueabihf GCC 6 & 
7. Ok to commit those?


Best regards,

Thomas

Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Kyrill Tkachov]

Hi Thomas,

On 09/04/18 15:29, Thomas Preudhomme wrote:

Hi Ramana,

On 06/04/18 17:17, Thomas Preudhomme wrote:
>
>
> On 06/04/18 17:08, Ramana Radhakrishnan wrote:
>> On 06/04/2018 16:54, Thomas Preudhomme wrote:
>>> Instruction pattern for setting the FPSCR expects the input value to be
>>> in a register. However, __builtin_arm_set_fpscr expander does not ensure
>>> that this is the case and as a result GCC ICEs when the builtin is
>>> called with a constant literal.
>>>
>>> This commit fixes the builtin to force the input value into a register.
>>> It also remove the unneeded volatile in the existing fpscr test and
>>> fixes the function prototype.
>>>
>>> ChangeLog entries are as follows:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2018-04-06  Thomas Preud'homme 
>>>
>>> PR target/85261
>>> * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
>>> into register.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2018-04-06  Thomas Preud'homme 
>>>
>>> PR target/85261
>>> * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
>>> literal value.  Expect 2 MCR instruction. Fix function prototype.
>>> Remove volatile keyword.
>>>
>>> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
>>> no regression.
>>>
>>> Is this ok for stage4?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>
>> (sorry about the duplicate for those who get it)
>>
>>
>> LGTM, though in this case I would prefer a bootstrap and regression run
>> as this is automatically exercised most with gcc.dg/atomic_*.c and you
>> really need this tested on linux than just bare-metal as I'm not sure
>> how this gets tested on arm-none-eabi.
>
> Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap
> right away.

Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4
--with-float=hard --enable-languages=c,c++,fortran --with-system-zlib
--enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any
regression either.

Ok to commit?



Thanks for doing this.
This is ok for trunk.


>
>>
>> What about earlier branches, have you looked ? This is a silly target
>> bug and fixes should go back to older branches in this particular case
>> after baking this on trunk for some time.
>
> GCC 6 and 7 are affected as well and a backport will be done once it has baked
> long enough of course.

Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that
is finished.


Thanks,
Kyrill



Best regards,

Thomas

Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Thomas Preudhomme]

Hi Ramana,

On 06/04/18 17:17, Thomas Preudhomme wrote:



On 06/04/18 17:08, Ramana Radhakrishnan wrote:

On 06/04/2018 16:54, Thomas Preudhomme wrote:

Instruction pattern for setting the FPSCR expects the input value to be
in a register. However, __builtin_arm_set_fpscr expander does not ensure
that this is the case and as a result GCC ICEs when the builtin is
called with a constant literal.

This commit fixes the builtin to force the input value into a register.
It also remove the unneeded volatile in the existing fpscr test and
fixes the function prototype.

ChangeLog entries are as follows:

*** gcc/ChangeLog ***

2018-04-06  Thomas Preud'homme  

PR target/85261
* config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
into register.

*** gcc/testsuite/ChangeLog ***

2018-04-06  Thomas Preud'homme  

PR target/85261
* gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
literal value.  Expect 2 MCR instruction.  Fix function prototype.
Remove volatile keyword.

Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
no regression.

Is this ok for stage4?

Best regards,

Thomas



(sorry about the duplicate for those who get it)


LGTM, though in this case I would prefer a bootstrap and regression run
as this is automatically exercised most with gcc.dg/atomic_*.c and you
really need this tested on linux than just bare-metal as I'm not sure
how this gets tested on arm-none-eabi.


Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap 
right away.


Done with --with-arch=armv8-a --with-mode=thumb --with-fpu=neon-vfpv4 
--with-float=hard --enable-languages=c,c++,fortran --with-system-zlib 
--enable-plugins --enable-bootstrap. Testsuite for that GCC does not show any 
regression either.


Ok to commit?





What about earlier branches, have you looked ? This is a silly target
bug and fixes should go back to older branches in this particular case
after baking this on trunk for some time.


GCC 6 and 7 are affected as well and a backport will be done once it has baked 
long enough of course.


Will now bootstrap and regtest against GCC 6 and 7. Will let you know once that 
is finished.


Best regards,

Thomas

Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Thomas Preudhomme]

On 06/04/18 17:08, Ramana Radhakrishnan wrote:

On 06/04/2018 16:54, Thomas Preudhomme wrote:

Instruction pattern for setting the FPSCR expects the input value to be
in a register. However, __builtin_arm_set_fpscr expander does not ensure
that this is the case and as a result GCC ICEs when the builtin is
called with a constant literal.

This commit fixes the builtin to force the input value into a register.
It also remove the unneeded volatile in the existing fpscr test and
fixes the function prototype.

ChangeLog entries are as follows:

*** gcc/ChangeLog ***

2018-04-06  Thomas Preud'homme  

PR target/85261
* config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
into register.

*** gcc/testsuite/ChangeLog ***

2018-04-06  Thomas Preud'homme  

PR target/85261
* gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
literal value.  Expect 2 MCR instruction.  Fix function prototype.
Remove volatile keyword.

Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
no regression.

Is this ok for stage4?

Best regards,

Thomas



(sorry about the duplicate for those who get it)


LGTM, though in this case I would prefer a bootstrap and regression run
as this is automatically exercised most with gcc.dg/atomic_*.c and you
really need this tested on linux than just bare-metal as I'm not sure
how this gets tested on arm-none-eabi.


Oh it is indeed. Didn't realized it was used anywhere. Will start bootstrap 
right away.




What about earlier branches, have you looked ? This is a silly target
bug and fixes should go back to older branches in this particular case
after baking this on trunk for some time.


GCC 6 and 7 are affected as well and a backport will be done once it has baked 
long enough of course.


Best regards,

Thomas

Re: [PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Ramana Radhakrishnan]

On 06/04/2018 16:54, Thomas Preudhomme wrote:
> Instruction pattern for setting the FPSCR expects the input value to be
> in a register. However, __builtin_arm_set_fpscr expander does not ensure
> that this is the case and as a result GCC ICEs when the builtin is
> called with a constant literal.
> 
> This commit fixes the builtin to force the input value into a register.
> It also remove the unneeded volatile in the existing fpscr test and
> fixes the function prototype.
> 
> ChangeLog entries are as follows:
> 
> *** gcc/ChangeLog ***
> 
> 2018-04-06  Thomas Preud'homme  
> 
>   PR target/85261
>   * config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
>   into register.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2018-04-06  Thomas Preud'homme  
> 
>   PR target/85261
>   * gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
>   literal value.  Expect 2 MCR instruction.  Fix function prototype.
>   Remove volatile keyword.
> 
> Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
> no regression.
> 
> Is this ok for stage4?
> 
> Best regards,
> 
> Thomas
> 

(sorry about the duplicate for those who get it)


LGTM, though in this case I would prefer a bootstrap and regression run
as this is automatically exercised most with gcc.dg/atomic_*.c and you
really need this tested on linux than just bare-metal as I'm not sure
how this gets tested on arm-none-eabi.

What about earlier branches, have you looked ? This is a silly target
bug and fixes should go back to older branches in this particular case
after baking this on trunk for some time.

regards
Ramana

[PATCH, GCC/ARM] Fix PR85261: ICE with FPSCR setter builtin

[Thomas Preudhomme]

Instruction pattern for setting the FPSCR expects the input value to be
in a register. However, __builtin_arm_set_fpscr expander does not ensure
that this is the case and as a result GCC ICEs when the builtin is
called with a constant literal.

This commit fixes the builtin to force the input value into a register.
It also remove the unneeded volatile in the existing fpscr test and
fixes the function prototype.

ChangeLog entries are as follows:

*** gcc/ChangeLog ***

2018-04-06  Thomas Preud'homme  

PR target/85261
* config/arm/arm-builtins.c (arm_expand_builtin): Force input operand
into register.

*** gcc/testsuite/ChangeLog ***

2018-04-06  Thomas Preud'homme  

PR target/85261
* gcc.target/arm/fpscr.c: Add call to __builtin_arm_set_fpscr with
literal value.  Expect 2 MCR instruction.  Fix function prototype.
Remove volatile keyword.

Testing: Built an arm-none-eabi GCC cross-compiler and testsuite shows
no regression.

Is this ok for stage4?

Best regards,

Thomas
diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
index 8940d1f6311bccf86664ab2eaa938735eec595f6..e100d933a77c5de4a13cb961d1bff40f57f2ea80 100644
--- a/gcc/config/arm/arm-builtins.c
+++ b/gcc/config/arm/arm-builtins.c
@@ -2592,7 +2592,7 @@ arm_expand_builtin (tree exp,
 	  icode = CODE_FOR_set_fpscr;
 	  arg0 = CALL_EXPR_ARG (exp, 0);
 	  op0 = expand_normal (arg0);
-	  pat = GEN_FCN (icode) (op0);
+	  pat = GEN_FCN (icode) (force_reg (SImode, op0));
 	}
   emit_insn (pat);
   return target;
diff --git a/gcc/testsuite/gcc.target/arm/fpscr.c b/gcc/testsuite/gcc.target/arm/fpscr.c
index 7b4d71d72d8964f6da0d0604bf59aeb4a895df43..4c3eaf7fcf75ad8582071ecb110fd1e4976a3b24 100644
--- a/gcc/testsuite/gcc.target/arm/fpscr.c
+++ b/gcc/testsuite/gcc.target/arm/fpscr.c
@@ -6,11 +6,14 @@
 /* { dg-add-options arm_fp } */
 
 void
-test_fpscr ()
+test_fpscr (void)
 {
-  volatile unsigned int status = __builtin_arm_get_fpscr ();
+  unsigned status;
+
+  __builtin_arm_set_fpscr (0);
+  status = __builtin_arm_get_fpscr ();
   __builtin_arm_set_fpscr (status);
 }
 
 /* { dg-final { scan-assembler "mrc\tp10, 7, r\[0-9\]+, cr1, cr0, 0" } } */
-/* { dg-final { scan-assembler "mcr\tp10, 7, r\[0-9\]+, cr1, cr0, 0" } } */
+/* { dg-final { scan-assembler-times "mcr\tp10, 7, r\[0-9\]+, cr1, cr0, 0" 2 } } */