[PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

Hi

The bug reported a particular test di-longlong64-sync-1.c failing when 
run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3] 
and -mthumb -march=armv6 -O[g,1,2,3].

According to what I could see, the crash was caused because of the 
explicit VOIDmode argument that was sent to emit_store_flag_force (). 
Since the comparing argument was a long long, it was being forced into a 
VOID type register before the comparison (in prepare_cmp_insn()) is done.

As pointed out by Kyrill, there is a comment on emit_store_flag() which 
says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs. 
  If it is VOIDmode, they cannot both be CONST_INT". This condition is 
not true in this case and thus I think it is suitable to change the 
argument.

Testing done: Checked for regressions on bootstrapped 
arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test 
cases.

Sudi

ChangeLog entries:

*** gcc/ChangeLog ***

2017-01-04  Sudakshina Das  <sudi.das@arm.com>

	PR target/82096
	* optabs.c (expand_atomic_compare_and_swap): Change argument
	to emit_store_flag_force.

*** gcc/testsuite/ChangeLog ***

2017-01-04  Sudakshina Das  <sudi.das@arm.com>

	PR target/82096
	* gcc.c-torture/compile/pr82096-1.c: New test.
	* gcc.c-torture/compile/pr82096-2.c: Likwise.

[-- Attachment #2: pr82096.diff --]
[-- Type: text/x-patch, Size: 1764 bytes --]

diff --git a/gcc/optabs.c b/gcc/optabs.c
index 225e955..45b018e 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -6295,7 +6295,7 @@ expand_atomic_compare_and_swap (rtx *ptarget_bool, rtx *ptarget_oval,
       if (cc_reg)
 	{
 	  target_bool = emit_store_flag_force (target_bool, EQ, cc_reg,
-					       const0_rtx, VOIDmode, 0, 1);
+					       const0_rtx, mode, 0, 1);
 	  goto success;
 	}
       goto success_bool_from_val;
@@ -6323,7 +6323,7 @@ expand_atomic_compare_and_swap (rtx *ptarget_bool, rtx *ptarget_oval,
 
  success_bool_from_val:
    target_bool = emit_store_flag_force (target_bool, EQ, target_oval,
-					expected, VOIDmode, 1, 1);
+					expected, mode, 1, 1);
  success:
   /* Make sure that the oval output winds up where the caller asked.  */
   if (ptarget_oval)
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c b/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c
new file mode 100644
index 0000000..07eb5f6
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { arm*-*-* } } } */
+/* { dg-options "-march=armv5t -mthumb -mfloat-abi=soft" } */
+
+static long long AL[24];
+
+int
+check_ok (void)
+{
+  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
+}
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c b/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c
new file mode 100644
index 0000000..2570b16
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { arm*-*-* } } } */
+/* { dg-options "-march=armv6 -mthumb -mfloat-abi=soft" } */
+
+static long long AL[24];
+
+int
+check_ok (void)
+{
+  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
+}

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Kyrill Tkachov]

Hi Sudi,

On 04/01/18 15:35, Sudakshina Das wrote:
> Hi
>
> The bug reported a particular test di-longlong64-sync-1.c failing when
> run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3]
> and -mthumb -march=armv6 -O[g,1,2,3].
>
> According to what I could see, the crash was caused because of the
> explicit VOIDmode argument that was sent to emit_store_flag_force ().
> Since the comparing argument was a long long, it was being forced into a
> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>
> As pointed out by Kyrill, there is a comment on emit_store_flag() which
> says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.
>   If it is VOIDmode, they cannot both be CONST_INT". This condition is
> not true in this case and thus I think it is suitable to change the
> argument.
>
> Testing done: Checked for regressions on bootstrapped
> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
> cases.
>
> Sudi
>
> ChangeLog entries:
>
> *** gcc/ChangeLog ***
>
> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>
>         PR target/82096
>         * optabs.c (expand_atomic_compare_and_swap): Change argument
>         to emit_store_flag_force.
>
> *** gcc/testsuite/ChangeLog ***
>
> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>
>         PR target/82096
>         * gcc.c-torture/compile/pr82096-1.c: New test.
>         * gcc.c-torture/compile/pr82096-2.c: Likwise.

diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c b/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c
new file mode 100644
index 0000000..07eb5f6
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { arm*-*-* } } } */
+/* { dg-options "-march=armv5t -mthumb -mfloat-abi=soft" } */

The tests in gcc.c-torture/compile/ are supposed to be target-independent, so
it's best to not gate it on target arm*-*-*. Best to add the arm-specific
options that you want using a dg-additional-options directive gated on target arm*-*-*.

+
+static long long AL[24];
+
+int
+check_ok (void)
+{
+  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
+}
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c b/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c
new file mode 100644
index 0000000..2570b16
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { arm*-*-* } } } */
+/* { dg-options "-march=armv6 -mthumb -mfloat-abi=soft" } */
+
+static long long AL[24];
+
+int
+check_ok (void)
+{
+  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
+}

I don't think we need an armv6 test here as the root cause is the same as on armv5t AFAICT
so this won't give us any extra code coverage.

Thanks,
Kyrill

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]

Hi Kyrill

On 04/01/18 16:36, Kyrill Tkachov wrote:
> Hi Sudi,
> 
> On 04/01/18 15:35, Sudakshina Das wrote:
>> Hi
>>
>> The bug reported a particular test di-longlong64-sync-1.c failing when
>> run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3]
>> and -mthumb -march=armv6 -O[g,1,2,3].
>>
>> According to what I could see, the crash was caused because of the
>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>> Since the comparing argument was a long long, it was being forced into a
>> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>>
>> As pointed out by Kyrill, there is a comment on emit_store_flag() which
>> says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.
>>   If it is VOIDmode, they cannot both be CONST_INT". This condition is
>> not true in this case and thus I think it is suitable to change the
>> argument.
>>
>> Testing done: Checked for regressions on bootstrapped
>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
>> cases.
>>
>> Sudi
>>
>> ChangeLog entries:
>>
>> *** gcc/ChangeLog ***
>>
>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>
>>         PR target/82096
>>         * optabs.c (expand_atomic_compare_and_swap): Change argument
>>         to emit_store_flag_force.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>
>>         PR target/82096
>>         * gcc.c-torture/compile/pr82096-1.c: New test.
>>         * gcc.c-torture/compile/pr82096-2.c: Likwise.
> 
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c 
> b/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c
> new file mode 100644
> index 0000000..07eb5f6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr82096-1.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target { arm*-*-* } } } */
> +/* { dg-options "-march=armv5t -mthumb -mfloat-abi=soft" } */
> 
> The tests in gcc.c-torture/compile/ are supposed to be 
> target-independent, so
> it's best to not gate it on target arm*-*-*. Best to add the arm-specific
> options that you want using a dg-additional-options directive gated on 
> target arm*-*-*.
> 
> +
> +static long long AL[24];
> +
> +int
> +check_ok (void)
> +{
> +  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 
> 0x1234567890ll));
> +}
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c 
> b/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c
> new file mode 100644
> index 0000000..2570b16
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr82096-2.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile { target { arm*-*-* } } } */
> +/* { dg-options "-march=armv6 -mthumb -mfloat-abi=soft" } */
> +
> +static long long AL[24];
> +
> +int
> +check_ok (void)
> +{
> +  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 
> 0x1234567890ll));
> +}
> 
> I don't think we need an armv6 test here as the root cause is the same 
> as on armv5t AFAICT
> so this won't give us any extra code coverage.
> 

I have made the changes in the test files as requested.

Thanks
Sudi

> Thanks,
> Kyrill
> 


[-- Attachment #2: pr82096.diff --]
[-- Type: text/x-patch, Size: 1250 bytes --]

diff --git a/gcc/optabs.c b/gcc/optabs.c
index 225e955..45b018e 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -6295,7 +6295,7 @@ expand_atomic_compare_and_swap (rtx *ptarget_bool, rtx *ptarget_oval,
       if (cc_reg)
 	{
 	  target_bool = emit_store_flag_force (target_bool, EQ, cc_reg,
-					       const0_rtx, VOIDmode, 0, 1);
+					       const0_rtx, mode, 0, 1);
 	  goto success;
 	}
       goto success_bool_from_val;
@@ -6323,7 +6323,7 @@ expand_atomic_compare_and_swap (rtx *ptarget_bool, rtx *ptarget_oval,
 
  success_bool_from_val:
    target_bool = emit_store_flag_force (target_bool, EQ, target_oval,
-					expected, VOIDmode, 1, 1);
+					expected, mode, 1, 1);
  success:
   /* Make sure that the oval output winds up where the caller asked.  */
   if (ptarget_oval)
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096.c b/gcc/testsuite/gcc.c-torture/compile/pr82096.c
new file mode 100644
index 0000000..9fed28c
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr82096.c
@@ -0,0 +1,9 @@
+/* { dg-additional-options "-march=armv5t -mthumb -mfloat-abi=soft" { target arm*-*-* } } */
+
+static long long AL[24];
+
+int
+check_ok (void)
+{
+  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
+}

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Jeff Law]

On 01/04/2018 08:35 AM, Sudakshina Das wrote:
> Hi
> 
> The bug reported a particular test di-longlong64-sync-1.c failing when
> run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3]
> and -mthumb -march=armv6 -O[g,1,2,3].
> 
> According to what I could see, the crash was caused because of the
> explicit VOIDmode argument that was sent to emit_store_flag_force ().
> Since the comparing argument was a long long, it was being forced into a
> VOID type register before the comparison (in prepare_cmp_insn()) is done.
> 
> As pointed out by Kyrill, there is a comment on emit_store_flag() which
> says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.
>  If it is VOIDmode, they cannot both be CONST_INT". This condition is
> not true in this case and thus I think it is suitable to change the
> argument.
> 
> Testing done: Checked for regressions on bootstrapped
> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
> cases.
> 
> Sudi
> 
> ChangeLog entries:
> 
> *** gcc/ChangeLog ***
> 
> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
> 
>     PR target/82096
>     * optabs.c (expand_atomic_compare_and_swap): Change argument
>     to emit_store_flag_force.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
> 
>     PR target/82096
>     * gcc.c-torture/compile/pr82096-1.c: New test.
>     * gcc.c-torture/compile/pr82096-2.c: Likwise.
In the case where both (op0/op1) to
emit_store_flag/emit_store_flag_force are constants, don't we know the
result of the comparison and shouldn't we have optimized the store flag
to something simpler?

I feel like I must be missing something here.

Jeff
> 

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

Hi Jeff

On 05/01/18 18:44, Jeff Law wrote:
> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>> Hi
>>
>> The bug reported a particular test di-longlong64-sync-1.c failing when
>> run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3]
>> and -mthumb -march=armv6 -O[g,1,2,3].
>>
>> According to what I could see, the crash was caused because of the
>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>> Since the comparing argument was a long long, it was being forced into a
>> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>>
>> As pointed out by Kyrill, there is a comment on emit_store_flag() which
>> says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.
>>   If it is VOIDmode, they cannot both be CONST_INT". This condition is
>> not true in this case and thus I think it is suitable to change the
>> argument.
>>
>> Testing done: Checked for regressions on bootstrapped
>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
>> cases.
>>
>> Sudi
>>
>> ChangeLog entries:
>>
>> *** gcc/ChangeLog ***
>>
>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>
>>      PR target/82096
>>      * optabs.c (expand_atomic_compare_and_swap): Change argument
>>      to emit_store_flag_force.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>
>>      PR target/82096
>>      * gcc.c-torture/compile/pr82096-1.c: New test.
>>      * gcc.c-torture/compile/pr82096-2.c: Likwise.
> In the case where both (op0/op1) to
> emit_store_flag/emit_store_flag_force are constants, don't we know the
> result of the comparison and shouldn't we have optimized the store flag
> to something simpler?
> 
> I feel like I must be missing something here.
> 

emit_store_flag_force () is comparing a register to op0.

The 2 constant arguments are to the expand_atomic_compare_and_swap () 
function. emit_store_flag_force () is used in case when this function is 
called by the bool variant of the built-in function where the bool 
return value is computed by comparing the result register with the 
expected op0.

Sudi

> Jeff
>>

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Jeff Law]

On 01/05/2018 12:25 PM, Sudakshina Das wrote:
> Hi Jeff
> 
> On 05/01/18 18:44, Jeff Law wrote:
>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>> Hi
>>>
>>> The bug reported a particular test di-longlong64-sync-1.c failing when
>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3]
>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>
>>> According to what I could see, the crash was caused because of the
>>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>>> Since the comparing argument was a long long, it was being forced into a
>>> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>>>
>>>
>>> As pointed out by Kyrill, there is a comment on emit_store_flag() which
>>> says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.
>>>   If it is VOIDmode, they cannot both be CONST_INT". This condition is
>>> not true in this case and thus I think it is suitable to change the
>>> argument.
>>>
>>> Testing done: Checked for regressions on bootstrapped
>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
>>> cases.
>>>
>>> Sudi
>>>
>>> ChangeLog entries:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>      PR target/82096
>>>      * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>      to emit_store_flag_force.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>      PR target/82096
>>>      * gcc.c-torture/compile/pr82096-1.c: New test.
>>>      * gcc.c-torture/compile/pr82096-2.c: Likwise.
>> In the case where both (op0/op1) to
>> emit_store_flag/emit_store_flag_force are constants, don't we know the
>> result of the comparison and shouldn't we have optimized the store flag
>> to something simpler?
>>
>> I feel like I must be missing something here.
>>
> 
> emit_store_flag_force () is comparing a register to op0.
?
/* Emit a store-flags instruction for comparison CODE on OP0 and OP1
   and storing in TARGET.  Normally return TARGET.
   Return 0 if that cannot be done.

   MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.  If
   it is VOIDmode, they cannot both be CONST_INT.


So we're comparing op0 and op1 AFAICT.  One, but not both can be a
CONST_INT.  If both are a CONST_INT, then you need to address the
problem in the caller (by optimizing away the condition).  If you've got
a REG and a CONST_INT, then the mode should be taken from the REG operand.





> 
> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
> function. emit_store_flag_force () is used in case when this function is
> called by the bool variant of the built-in function where the bool
> return value is computed by comparing the result register with the
> expected op0.
So if only one of the two objects is a CONST_INT, then the mode should
come from the other object.  I think that's the fundamental problem here
and that you're just papering over it by changing the caller.



For example in emit_store_flag_1 we have this code:

  /* If one operand is constant, make it the second one.  Only do this
     if the other operand is not constant as well.  */

  if (swap_commutative_operands_p (op0, op1))
    {
      std::swap (op0, op1);
      code = swap_condition (code);
    }

  if (mode == VOIDmode)
    mode = GET_MODE (op0);

I think if you do this in emit_store_flag_force as well everything will
"just work".

You can put it after this call/test pair:

 /* First see if emit_store_flag can do the job.  */
  tem = emit_store_flag (target, code, op0, op1, mode, unsignedp,
normalizep);
  if (tem != 0)
    return tem;


jeff

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

Hi Jeff

On 09/01/18 23:43, Jeff Law wrote:
> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>> Hi Jeff
>>
>> On 05/01/18 18:44, Jeff Law wrote:
>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>> Hi
>>>>
>>>> The bug reported a particular test di-longlong64-sync-1.c failing when
>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t -O[g,1,2,3]
>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>
>>>> According to what I could see, the crash was caused because of the
>>>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>>>> Since the comparing argument was a long long, it was being forced into a
>>>> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>>>>
>>>>
>>>> As pointed out by Kyrill, there is a comment on emit_store_flag() which
>>>> says "MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.
>>>>    If it is VOIDmode, they cannot both be CONST_INT". This condition is
>>>> not true in this case and thus I think it is suitable to change the
>>>> argument.
>>>>
>>>> Testing done: Checked for regressions on bootstrapped
>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
>>>> cases.
>>>>
>>>> Sudi
>>>>
>>>> ChangeLog entries:
>>>>
>>>> *** gcc/ChangeLog ***
>>>>
>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>
>>>>       PR target/82096
>>>>       * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>       to emit_store_flag_force.
>>>>
>>>> *** gcc/testsuite/ChangeLog ***
>>>>
>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>
>>>>       PR target/82096
>>>>       * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>       * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>> In the case where both (op0/op1) to
>>> emit_store_flag/emit_store_flag_force are constants, don't we know the
>>> result of the comparison and shouldn't we have optimized the store flag
>>> to something simpler?
>>>
>>> I feel like I must be missing something here.
>>>
>>
>> emit_store_flag_force () is comparing a register to op0.
> ?
> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>     and storing in TARGET.  Normally return TARGET.
>     Return 0 if that cannot be done.
> 
>     MODE is the mode to use for OP0 and OP1 should they be CONST_INTs.  If
>     it is VOIDmode, they cannot both be CONST_INT.
> 
> 
> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
> CONST_INT.  If both are a CONST_INT, then you need to address the
> problem in the caller (by optimizing away the condition).  If you've got
> a REG and a CONST_INT, then the mode should be taken from the REG operand.
> 
> 
> 
> 
> 
>>
>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>> function. emit_store_flag_force () is used in case when this function is
>> called by the bool variant of the built-in function where the bool
>> return value is computed by comparing the result register with the
>> expected op0.
> So if only one of the two objects is a CONST_INT, then the mode should
> come from the other object.  I think that's the fundamental problem here
> and that you're just papering over it by changing the caller.
> 
I think my earlier explanation was a bit misleading and I may have 
rushed into quoting the comment about both operands being const for 
emit_store_flag_force(). The problem is with the function and I do agree 
with your suggestion of changing the function to add the code below to 
be a better approach than the changing the caller. I will change the 
patch and test it.

Thanks
Sudi
> 
> 
> For example in emit_store_flag_1 we have this code:
> 
>    /* If one operand is constant, make it the second one.  Only do this
>       if the other operand is not constant as well.  */
> 
>    if (swap_commutative_operands_p (op0, op1))
>      {
>        std::swap (op0, op1);
>        code = swap_condition (code);
>      }
> 
>    if (mode == VOIDmode)
>      mode = GET_MODE (op0);
> 
> I think if you do this in emit_store_flag_force as well everything will
> "just work".
> 
> You can put it after this call/test pair:
> 
>   /* First see if emit_store_flag can do the job.  */
>    tem = emit_store_flag (target, code, op0, op1, mode, unsignedp,
> normalizep);
>    if (tem != 0)
>      return tem;
> 
> 
> jeff
> 

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

[-- Attachment #1: Type: text/plain, Size: 5460 bytes --]

Hi Jeff

On 10/01/18 10:44, Sudakshina Das wrote:
> Hi Jeff
> 
> On 09/01/18 23:43, Jeff Law wrote:
>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>> Hi Jeff
>>>
>>> On 05/01/18 18:44, Jeff Law wrote:
>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>> Hi
>>>>>
>>>>> The bug reported a particular test di-longlong64-sync-1.c failing when
>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t 
>>>>> -O[g,1,2,3]
>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>
>>>>> According to what I could see, the crash was caused because of the
>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>>>>> Since the comparing argument was a long long, it was being forced 
>>>>> into a
>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is done. 
>>>>>
>>>>>
>>>>>
>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag() 
>>>>> which
>>>>> says "MODE is the mode to use for OP0 and OP1 should they be 
>>>>> CONST_INTs.
>>>>>    If it is VOIDmode, they cannot both be CONST_INT". This 
>>>>> condition is
>>>>> not true in this case and thus I think it is suitable to change the
>>>>> argument.
>>>>>
>>>>> Testing done: Checked for regressions on bootstrapped
>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new test
>>>>> cases.
>>>>>
>>>>> Sudi
>>>>>
>>>>> ChangeLog entries:
>>>>>
>>>>> *** gcc/ChangeLog ***
>>>>>
>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>
>>>>>       PR target/82096
>>>>>       * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>       to emit_store_flag_force.
>>>>>
>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>
>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>
>>>>>       PR target/82096
>>>>>       * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>       * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>> In the case where both (op0/op1) to
>>>> emit_store_flag/emit_store_flag_force are constants, don't we know the
>>>> result of the comparison and shouldn't we have optimized the store flag
>>>> to something simpler?
>>>>
>>>> I feel like I must be missing something here.
>>>>
>>>
>>> emit_store_flag_force () is comparing a register to op0.
>> ?
>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>     and storing in TARGET.  Normally return TARGET.
>>     Return 0 if that cannot be done.
>>
>>     MODE is the mode to use for OP0 and OP1 should they be 
>> CONST_INTs.  If
>>     it is VOIDmode, they cannot both be CONST_INT.
>>
>>
>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>> CONST_INT.  If both are a CONST_INT, then you need to address the
>> problem in the caller (by optimizing away the condition).  If you've got
>> a REG and a CONST_INT, then the mode should be taken from the REG 
>> operand.
>>
>>
>>
>>
>>
>>>
>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>> function. emit_store_flag_force () is used in case when this function is
>>> called by the bool variant of the built-in function where the bool
>>> return value is computed by comparing the result register with the
>>> expected op0.
>> So if only one of the two objects is a CONST_INT, then the mode should
>> come from the other object.  I think that's the fundamental problem here
>> and that you're just papering over it by changing the caller.
>>
> I think my earlier explanation was a bit misleading and I may have 
> rushed into quoting the comment about both operands being const for 
> emit_store_flag_force(). The problem is with the function and I do agree 
> with your suggestion of changing the function to add the code below to 
> be a better approach than the changing the caller. I will change the 
> patch and test it.
> 

This is the updated patch according to your suggestions.

Testing: Checked for regressions on arm-none-linux-gnueabihf and added 
new test case.

Thanks
Sudi

ChangeLog entries:

*** gcc/ChangeLog ***

2017-01-10  Sudakshina Das  <sudi.das@arm.com>

	PR target/82096
	* expmed.c (emit_store_flag_force): Swap if const op0
	and change VOIDmode to mode of op0.

*** gcc/testsuite/ChangeLog ***

2017-01-10  Sudakshina Das  <sudi.das@arm.com>

	PR target/82096
	* gcc.c-torture/compile/pr82096.c: New test.


> Thanks
> Sudi
>>
>>
>> For example in emit_store_flag_1 we have this code:
>>
>>    /* If one operand is constant, make it the second one.  Only do this
>>       if the other operand is not constant as well.  */
>>
>>    if (swap_commutative_operands_p (op0, op1))
>>      {
>>        std::swap (op0, op1);
>>        code = swap_condition (code);
>>      }
>>
>>    if (mode == VOIDmode)
>>      mode = GET_MODE (op0);
>>
>> I think if you do this in emit_store_flag_force as well everything will
>> "just work".
>>
>> You can put it after this call/test pair:
>>
>>   /* First see if emit_store_flag can do the job.  */
>>    tem = emit_store_flag (target, code, op0, op1, mode, unsignedp,
>> normalizep);
>>    if (tem != 0)
>>      return tem;
>>
>>
>> jeff
>>
> 


[-- Attachment #2: pr82096.diff --]
[-- Type: text/x-patch, Size: 1077 bytes --]

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 6b22946..142d542 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -6084,6 +6084,18 @@ emit_store_flag_force (rtx target, enum rtx_code code, rtx op0, rtx op1,
   if (tem != 0)
     return tem;
 
+  /* If one operand is constant, make it the second one.  Only do this
+     if the other operand is not constant as well.  */
+
+  if (swap_commutative_operands_p (op0, op1))
+    {
+      std::swap (op0, op1);
+      code = swap_condition (code);
+    }
+
+  if (mode == VOIDmode)
+    mode = GET_MODE (op0);
+
   if (!target)
     target = gen_reg_rtx (word_mode);
 
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr82096.c b/gcc/testsuite/gcc.c-torture/compile/pr82096.c
new file mode 100644
index 0000000..9fed28c
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr82096.c
@@ -0,0 +1,9 @@
+/* { dg-additional-options "-march=armv5t -mthumb -mfloat-abi=soft" { target arm*-*-* } } */
+
+static long long AL[24];
+
+int
+check_ok (void)
+{
+  return (__sync_bool_compare_and_swap (AL+1, 0x200000003ll, 0x1234567890ll));
+}

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Jeff Law]

On 01/10/2018 09:25 AM, Sudakshina Das wrote:
> Hi Jeff
> 
> On 10/01/18 10:44, Sudakshina Das wrote:
>> Hi Jeff
>>
>> On 09/01/18 23:43, Jeff Law wrote:
>>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>>> Hi Jeff
>>>>
>>>> On 05/01/18 18:44, Jeff Law wrote:
>>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>>> Hi
>>>>>>
>>>>>> The bug reported a particular test di-longlong64-sync-1.c failing
>>>>>> when
>>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t
>>>>>> -O[g,1,2,3]
>>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>>
>>>>>> According to what I could see, the crash was caused because of the
>>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>>>>>> Since the comparing argument was a long long, it was being forced
>>>>>> into a
>>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>>>>>>
>>>>>>
>>>>>>
>>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag()
>>>>>> which
>>>>>> says "MODE is the mode to use for OP0 and OP1 should they be
>>>>>> CONST_INTs.
>>>>>>    If it is VOIDmode, they cannot both be CONST_INT". This
>>>>>> condition is
>>>>>> not true in this case and thus I think it is suitable to change the
>>>>>> argument.
>>>>>>
>>>>>> Testing done: Checked for regressions on bootstrapped
>>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
>>>>>> test
>>>>>> cases.
>>>>>>
>>>>>> Sudi
>>>>>>
>>>>>> ChangeLog entries:
>>>>>>
>>>>>> *** gcc/ChangeLog ***
>>>>>>
>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>
>>>>>>       PR target/82096
>>>>>>       * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>>       to emit_store_flag_force.
>>>>>>
>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>
>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>
>>>>>>       PR target/82096
>>>>>>       * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>>       * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>>> In the case where both (op0/op1) to
>>>>> emit_store_flag/emit_store_flag_force are constants, don't we know the
>>>>> result of the comparison and shouldn't we have optimized the store
>>>>> flag
>>>>> to something simpler?
>>>>>
>>>>> I feel like I must be missing something here.
>>>>>
>>>>
>>>> emit_store_flag_force () is comparing a register to op0.
>>> ?
>>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>>     and storing in TARGET.  Normally return TARGET.
>>>     Return 0 if that cannot be done.
>>>
>>>     MODE is the mode to use for OP0 and OP1 should they be
>>> CONST_INTs.  If
>>>     it is VOIDmode, they cannot both be CONST_INT.
>>>
>>>
>>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>>> CONST_INT.  If both are a CONST_INT, then you need to address the
>>> problem in the caller (by optimizing away the condition).  If you've got
>>> a REG and a CONST_INT, then the mode should be taken from the REG
>>> operand.
>>>
>>>
>>>
>>>
>>>
>>>>
>>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>>> function. emit_store_flag_force () is used in case when this
>>>> function is
>>>> called by the bool variant of the built-in function where the bool
>>>> return value is computed by comparing the result register with the
>>>> expected op0.
>>> So if only one of the two objects is a CONST_INT, then the mode should
>>> come from the other object.  I think that's the fundamental problem here
>>> and that you're just papering over it by changing the caller.
>>>
>> I think my earlier explanation was a bit misleading and I may have
>> rushed into quoting the comment about both operands being const for
>> emit_store_flag_force(). The problem is with the function and I do
>> agree with your suggestion of changing the function to add the code
>> below to be a better approach than the changing the caller. I will
>> change the patch and test it.
>>
> 
> This is the updated patch according to your suggestions.
> 
> Testing: Checked for regressions on arm-none-linux-gnueabihf and added
> new test case.
> 
> Thanks
> Sudi
> 
> ChangeLog entries:
> 
> *** gcc/ChangeLog ***
> 
> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
> 
>     PR target/82096
>     * expmed.c (emit_store_flag_force): Swap if const op0
>     and change VOIDmode to mode of op0.
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
> 
>     PR target/82096
>     * gcc.c-torture/compile/pr82096.c: New test.
OK.
jeff

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

Hi Jeff

On 10/01/18 21:08, Jeff Law wrote:
> On 01/10/2018 09:25 AM, Sudakshina Das wrote:
>> Hi Jeff
>>
>> On 10/01/18 10:44, Sudakshina Das wrote:
>>> Hi Jeff
>>>
>>> On 09/01/18 23:43, Jeff Law wrote:
>>>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>>>> Hi Jeff
>>>>>
>>>>> On 05/01/18 18:44, Jeff Law wrote:
>>>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> The bug reported a particular test di-longlong64-sync-1.c failing
>>>>>>> when
>>>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t
>>>>>>> -O[g,1,2,3]
>>>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>>>
>>>>>>> According to what I could see, the crash was caused because of the
>>>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force ().
>>>>>>> Since the comparing argument was a long long, it was being forced
>>>>>>> into a
>>>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is done.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag()
>>>>>>> which
>>>>>>> says "MODE is the mode to use for OP0 and OP1 should they be
>>>>>>> CONST_INTs.
>>>>>>>     If it is VOIDmode, they cannot both be CONST_INT". This
>>>>>>> condition is
>>>>>>> not true in this case and thus I think it is suitable to change the
>>>>>>> argument.
>>>>>>>
>>>>>>> Testing done: Checked for regressions on bootstrapped
>>>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
>>>>>>> test
>>>>>>> cases.
>>>>>>>
>>>>>>> Sudi
>>>>>>>
>>>>>>> ChangeLog entries:
>>>>>>>
>>>>>>> *** gcc/ChangeLog ***
>>>>>>>
>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>
>>>>>>>        PR target/82096
>>>>>>>        * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>>>        to emit_store_flag_force.
>>>>>>>
>>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>>
>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>
>>>>>>>        PR target/82096
>>>>>>>        * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>>>        * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>>>> In the case where both (op0/op1) to
>>>>>> emit_store_flag/emit_store_flag_force are constants, don't we know the
>>>>>> result of the comparison and shouldn't we have optimized the store
>>>>>> flag
>>>>>> to something simpler?
>>>>>>
>>>>>> I feel like I must be missing something here.
>>>>>>
>>>>>
>>>>> emit_store_flag_force () is comparing a register to op0.
>>>> ?
>>>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>>>      and storing in TARGET.  Normally return TARGET.
>>>>      Return 0 if that cannot be done.
>>>>
>>>>      MODE is the mode to use for OP0 and OP1 should they be
>>>> CONST_INTs.  If
>>>>      it is VOIDmode, they cannot both be CONST_INT.
>>>>
>>>>
>>>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>>>> CONST_INT.  If both are a CONST_INT, then you need to address the
>>>> problem in the caller (by optimizing away the condition).  If you've got
>>>> a REG and a CONST_INT, then the mode should be taken from the REG
>>>> operand.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>>>> function. emit_store_flag_force () is used in case when this
>>>>> function is
>>>>> called by the bool variant of the built-in function where the bool
>>>>> return value is computed by comparing the result register with the
>>>>> expected op0.
>>>> So if only one of the two objects is a CONST_INT, then the mode should
>>>> come from the other object.  I think that's the fundamental problem here
>>>> and that you're just papering over it by changing the caller.
>>>>
>>> I think my earlier explanation was a bit misleading and I may have
>>> rushed into quoting the comment about both operands being const for
>>> emit_store_flag_force(). The problem is with the function and I do
>>> agree with your suggestion of changing the function to add the code
>>> below to be a better approach than the changing the caller. I will
>>> change the patch and test it.
>>>
>>
>> This is the updated patch according to your suggestions.
>>
>> Testing: Checked for regressions on arm-none-linux-gnueabihf and added
>> new test case.
>>
>> Thanks
>> Sudi
>>
>> ChangeLog entries:
>>
>> *** gcc/ChangeLog ***
>>
>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>
>>      PR target/82096
>>      * expmed.c (emit_store_flag_force): Swap if const op0
>>      and change VOIDmode to mode of op0.
>>
>> *** gcc/testsuite/ChangeLog ***
>>
>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>
>>      PR target/82096
>>      * gcc.c-torture/compile/pr82096.c: New test.
> OK.

Thanks. Committed as r256526.
Sudi

> jeff
> 

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Christophe Lyon]

Hi,

On 11 January 2018 at 11:58, Sudakshina Das <sudi.das@arm.com> wrote:
> Hi Jeff
>
>
> On 10/01/18 21:08, Jeff Law wrote:
>>
>> On 01/10/2018 09:25 AM, Sudakshina Das wrote:
>>>
>>> Hi Jeff
>>>
>>> On 10/01/18 10:44, Sudakshina Das wrote:
>>>>
>>>> Hi Jeff
>>>>
>>>> On 09/01/18 23:43, Jeff Law wrote:
>>>>>
>>>>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>>>>>
>>>>>> Hi Jeff
>>>>>>
>>>>>> On 05/01/18 18:44, Jeff Law wrote:
>>>>>>>
>>>>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> The bug reported a particular test di-longlong64-sync-1.c failing
>>>>>>>> when
>>>>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t
>>>>>>>> -O[g,1,2,3]
>>>>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>>>>
>>>>>>>> According to what I could see, the crash was caused because of the
>>>>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force
>>>>>>>> ().
>>>>>>>> Since the comparing argument was a long long, it was being forced
>>>>>>>> into a
>>>>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is
>>>>>>>> done.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag()
>>>>>>>> which
>>>>>>>> says "MODE is the mode to use for OP0 and OP1 should they be
>>>>>>>> CONST_INTs.
>>>>>>>>     If it is VOIDmode, they cannot both be CONST_INT". This
>>>>>>>> condition is
>>>>>>>> not true in this case and thus I think it is suitable to change the
>>>>>>>> argument.
>>>>>>>>
>>>>>>>> Testing done: Checked for regressions on bootstrapped
>>>>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
>>>>>>>> test
>>>>>>>> cases.
>>>>>>>>
>>>>>>>> Sudi
>>>>>>>>
>>>>>>>> ChangeLog entries:
>>>>>>>>
>>>>>>>> *** gcc/ChangeLog ***
>>>>>>>>
>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>
>>>>>>>>        PR target/82096
>>>>>>>>        * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>>>>        to emit_store_flag_force.
>>>>>>>>
>>>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>>>
>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>
>>>>>>>>        PR target/82096
>>>>>>>>        * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>>>>        * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>>>>>
>>>>>>> In the case where both (op0/op1) to
>>>>>>> emit_store_flag/emit_store_flag_force are constants, don't we know
>>>>>>> the
>>>>>>> result of the comparison and shouldn't we have optimized the store
>>>>>>> flag
>>>>>>> to something simpler?
>>>>>>>
>>>>>>> I feel like I must be missing something here.
>>>>>>>
>>>>>>
>>>>>> emit_store_flag_force () is comparing a register to op0.
>>>>>
>>>>> ?
>>>>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>>>>      and storing in TARGET.  Normally return TARGET.
>>>>>      Return 0 if that cannot be done.
>>>>>
>>>>>      MODE is the mode to use for OP0 and OP1 should they be
>>>>> CONST_INTs.  If
>>>>>      it is VOIDmode, they cannot both be CONST_INT.
>>>>>
>>>>>
>>>>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>>>>> CONST_INT.  If both are a CONST_INT, then you need to address the
>>>>> problem in the caller (by optimizing away the condition).  If you've
>>>>> got
>>>>> a REG and a CONST_INT, then the mode should be taken from the REG
>>>>> operand.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>>>>> function. emit_store_flag_force () is used in case when this
>>>>>> function is
>>>>>> called by the bool variant of the built-in function where the bool
>>>>>> return value is computed by comparing the result register with the
>>>>>> expected op0.
>>>>>
>>>>> So if only one of the two objects is a CONST_INT, then the mode should
>>>>> come from the other object.  I think that's the fundamental problem
>>>>> here
>>>>> and that you're just papering over it by changing the caller.
>>>>>
>>>> I think my earlier explanation was a bit misleading and I may have
>>>> rushed into quoting the comment about both operands being const for
>>>> emit_store_flag_force(). The problem is with the function and I do
>>>> agree with your suggestion of changing the function to add the code
>>>> below to be a better approach than the changing the caller. I will
>>>> change the patch and test it.
>>>>
>>>
>>> This is the updated patch according to your suggestions.
>>>
>>> Testing: Checked for regressions on arm-none-linux-gnueabihf and added
>>> new test case.
>>>
>>> Thanks
>>> Sudi
>>>
>>> ChangeLog entries:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>      PR target/82096
>>>      * expmed.c (emit_store_flag_force): Swap if const op0
>>>      and change VOIDmode to mode of op0.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>      PR target/82096
>>>      * gcc.c-torture/compile/pr82096.c: New test.
>>
>> OK.
>
>
> Thanks. Committed as r256526.
> Sudi
>

Could you add a guard like in other tests to skip it if the user added
-mfloat-abi=XXX when running the tests?

For instance, I have a configuration where I add
-mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
and the new test fails because:
xgcc: error: -mfloat-abi=soft and -mfloat-abi=hard may not be used together

Thanks

Christophe

>> jeff
>>
>

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Jeff Law]

On 01/12/2018 01:45 AM, Christophe Lyon wrote:
> Hi,
> 
> On 11 January 2018 at 11:58, Sudakshina Das <sudi.das@arm.com> wrote:
>> Hi Jeff
>>
>>
>> On 10/01/18 21:08, Jeff Law wrote:
>>>
>>> On 01/10/2018 09:25 AM, Sudakshina Das wrote:
>>>>
>>>> Hi Jeff
>>>>
>>>> On 10/01/18 10:44, Sudakshina Das wrote:
>>>>>
>>>>> Hi Jeff
>>>>>
>>>>> On 09/01/18 23:43, Jeff Law wrote:
>>>>>>
>>>>>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>>>>>>
>>>>>>> Hi Jeff
>>>>>>>
>>>>>>> On 05/01/18 18:44, Jeff Law wrote:
>>>>>>>>
>>>>>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> The bug reported a particular test di-longlong64-sync-1.c failing
>>>>>>>>> when
>>>>>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t
>>>>>>>>> -O[g,1,2,3]
>>>>>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>>>>>
>>>>>>>>> According to what I could see, the crash was caused because of the
>>>>>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force
>>>>>>>>> ().
>>>>>>>>> Since the comparing argument was a long long, it was being forced
>>>>>>>>> into a
>>>>>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is
>>>>>>>>> done.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag()
>>>>>>>>> which
>>>>>>>>> says "MODE is the mode to use for OP0 and OP1 should they be
>>>>>>>>> CONST_INTs.
>>>>>>>>>     If it is VOIDmode, they cannot both be CONST_INT". This
>>>>>>>>> condition is
>>>>>>>>> not true in this case and thus I think it is suitable to change the
>>>>>>>>> argument.
>>>>>>>>>
>>>>>>>>> Testing done: Checked for regressions on bootstrapped
>>>>>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
>>>>>>>>> test
>>>>>>>>> cases.
>>>>>>>>>
>>>>>>>>> Sudi
>>>>>>>>>
>>>>>>>>> ChangeLog entries:
>>>>>>>>>
>>>>>>>>> *** gcc/ChangeLog ***
>>>>>>>>>
>>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>>
>>>>>>>>>        PR target/82096
>>>>>>>>>        * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>>>>>        to emit_store_flag_force.
>>>>>>>>>
>>>>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>>>>
>>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>>
>>>>>>>>>        PR target/82096
>>>>>>>>>        * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>>>>>        * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>>>>>>
>>>>>>>> In the case where both (op0/op1) to
>>>>>>>> emit_store_flag/emit_store_flag_force are constants, don't we know
>>>>>>>> the
>>>>>>>> result of the comparison and shouldn't we have optimized the store
>>>>>>>> flag
>>>>>>>> to something simpler?
>>>>>>>>
>>>>>>>> I feel like I must be missing something here.
>>>>>>>>
>>>>>>>
>>>>>>> emit_store_flag_force () is comparing a register to op0.
>>>>>>
>>>>>> ?
>>>>>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>>>>>      and storing in TARGET.  Normally return TARGET.
>>>>>>      Return 0 if that cannot be done.
>>>>>>
>>>>>>      MODE is the mode to use for OP0 and OP1 should they be
>>>>>> CONST_INTs.  If
>>>>>>      it is VOIDmode, they cannot both be CONST_INT.
>>>>>>
>>>>>>
>>>>>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>>>>>> CONST_INT.  If both are a CONST_INT, then you need to address the
>>>>>> problem in the caller (by optimizing away the condition).  If you've
>>>>>> got
>>>>>> a REG and a CONST_INT, then the mode should be taken from the REG
>>>>>> operand.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>>>>>> function. emit_store_flag_force () is used in case when this
>>>>>>> function is
>>>>>>> called by the bool variant of the built-in function where the bool
>>>>>>> return value is computed by comparing the result register with the
>>>>>>> expected op0.
>>>>>>
>>>>>> So if only one of the two objects is a CONST_INT, then the mode should
>>>>>> come from the other object.  I think that's the fundamental problem
>>>>>> here
>>>>>> and that you're just papering over it by changing the caller.
>>>>>>
>>>>> I think my earlier explanation was a bit misleading and I may have
>>>>> rushed into quoting the comment about both operands being const for
>>>>> emit_store_flag_force(). The problem is with the function and I do
>>>>> agree with your suggestion of changing the function to add the code
>>>>> below to be a better approach than the changing the caller. I will
>>>>> change the patch and test it.
>>>>>
>>>>
>>>> This is the updated patch according to your suggestions.
>>>>
>>>> Testing: Checked for regressions on arm-none-linux-gnueabihf and added
>>>> new test case.
>>>>
>>>> Thanks
>>>> Sudi
>>>>
>>>> ChangeLog entries:
>>>>
>>>> *** gcc/ChangeLog ***
>>>>
>>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>>
>>>>      PR target/82096
>>>>      * expmed.c (emit_store_flag_force): Swap if const op0
>>>>      and change VOIDmode to mode of op0.
>>>>
>>>> *** gcc/testsuite/ChangeLog ***
>>>>
>>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>>
>>>>      PR target/82096
>>>>      * gcc.c-torture/compile/pr82096.c: New test.
>>>
>>> OK.
>>
>>
>> Thanks. Committed as r256526.
>> Sudi
>>
> 
> Could you add a guard like in other tests to skip it if the user added
> -mfloat-abi=XXX when running the tests?
> 
> For instance, I have a configuration where I add
> -mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
> and the new test fails because:
> xgcc: error: -mfloat-abi=soft and -mfloat-abi=hard may not be used together
It's starting to feel like the test should move into gcc.target/arm :-)
 I nearly suggested that already.  Consider moving it into
gcc.target/arm pre-approved along with adding the -O<whatever you need>
to the options and whatever is needed to skip the test at the
appropriate time.


jeff

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

[Sudakshina Das]

Hi Jeff

On 12/01/18 23:00, Jeff Law wrote:
> On 01/12/2018 01:45 AM, Christophe Lyon wrote:
>> Hi,
>>
>> On 11 January 2018 at 11:58, Sudakshina Das <sudi.das@arm.com> wrote:
>>> Hi Jeff
>>>
>>>
>>> On 10/01/18 21:08, Jeff Law wrote:
>>>>
>>>> On 01/10/2018 09:25 AM, Sudakshina Das wrote:
>>>>>
>>>>> Hi Jeff
>>>>>
>>>>> On 10/01/18 10:44, Sudakshina Das wrote:
>>>>>>
>>>>>> Hi Jeff
>>>>>>
>>>>>> On 09/01/18 23:43, Jeff Law wrote:
>>>>>>>
>>>>>>> On 01/05/2018 12:25 PM, Sudakshina Das wrote:
>>>>>>>>
>>>>>>>> Hi Jeff
>>>>>>>>
>>>>>>>> On 05/01/18 18:44, Jeff Law wrote:
>>>>>>>>>
>>>>>>>>> On 01/04/2018 08:35 AM, Sudakshina Das wrote:
>>>>>>>>>>
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> The bug reported a particular test di-longlong64-sync-1.c failing
>>>>>>>>>> when
>>>>>>>>>> run on arm-linux-gnueabi with options -mthumb -march=armv5t
>>>>>>>>>> -O[g,1,2,3]
>>>>>>>>>> and -mthumb -march=armv6 -O[g,1,2,3].
>>>>>>>>>>
>>>>>>>>>> According to what I could see, the crash was caused because of the
>>>>>>>>>> explicit VOIDmode argument that was sent to emit_store_flag_force
>>>>>>>>>> ().
>>>>>>>>>> Since the comparing argument was a long long, it was being forced
>>>>>>>>>> into a
>>>>>>>>>> VOID type register before the comparison (in prepare_cmp_insn()) is
>>>>>>>>>> done.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> As pointed out by Kyrill, there is a comment on emit_store_flag()
>>>>>>>>>> which
>>>>>>>>>> says "MODE is the mode to use for OP0 and OP1 should they be
>>>>>>>>>> CONST_INTs.
>>>>>>>>>>      If it is VOIDmode, they cannot both be CONST_INT". This
>>>>>>>>>> condition is
>>>>>>>>>> not true in this case and thus I think it is suitable to change the
>>>>>>>>>> argument.
>>>>>>>>>>
>>>>>>>>>> Testing done: Checked for regressions on bootstrapped
>>>>>>>>>> arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
>>>>>>>>>> test
>>>>>>>>>> cases.
>>>>>>>>>>
>>>>>>>>>> Sudi
>>>>>>>>>>
>>>>>>>>>> ChangeLog entries:
>>>>>>>>>>
>>>>>>>>>> *** gcc/ChangeLog ***
>>>>>>>>>>
>>>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>>>
>>>>>>>>>>         PR target/82096
>>>>>>>>>>         * optabs.c (expand_atomic_compare_and_swap): Change argument
>>>>>>>>>>         to emit_store_flag_force.
>>>>>>>>>>
>>>>>>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>>>>>>
>>>>>>>>>> 2017-01-04  Sudakshina Das  <sudi.das@arm.com>
>>>>>>>>>>
>>>>>>>>>>         PR target/82096
>>>>>>>>>>         * gcc.c-torture/compile/pr82096-1.c: New test.
>>>>>>>>>>         * gcc.c-torture/compile/pr82096-2.c: Likwise.
>>>>>>>>>
>>>>>>>>> In the case where both (op0/op1) to
>>>>>>>>> emit_store_flag/emit_store_flag_force are constants, don't we know
>>>>>>>>> the
>>>>>>>>> result of the comparison and shouldn't we have optimized the store
>>>>>>>>> flag
>>>>>>>>> to something simpler?
>>>>>>>>>
>>>>>>>>> I feel like I must be missing something here.
>>>>>>>>>
>>>>>>>>
>>>>>>>> emit_store_flag_force () is comparing a register to op0.
>>>>>>>
>>>>>>> ?
>>>>>>> /* Emit a store-flags instruction for comparison CODE on OP0 and OP1
>>>>>>>       and storing in TARGET.  Normally return TARGET.
>>>>>>>       Return 0 if that cannot be done.
>>>>>>>
>>>>>>>       MODE is the mode to use for OP0 and OP1 should they be
>>>>>>> CONST_INTs.  If
>>>>>>>       it is VOIDmode, they cannot both be CONST_INT.
>>>>>>>
>>>>>>>
>>>>>>> So we're comparing op0 and op1 AFAICT.  One, but not both can be a
>>>>>>> CONST_INT.  If both are a CONST_INT, then you need to address the
>>>>>>> problem in the caller (by optimizing away the condition).  If you've
>>>>>>> got
>>>>>>> a REG and a CONST_INT, then the mode should be taken from the REG
>>>>>>> operand.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> The 2 constant arguments are to the expand_atomic_compare_and_swap ()
>>>>>>>> function. emit_store_flag_force () is used in case when this
>>>>>>>> function is
>>>>>>>> called by the bool variant of the built-in function where the bool
>>>>>>>> return value is computed by comparing the result register with the
>>>>>>>> expected op0.
>>>>>>>
>>>>>>> So if only one of the two objects is a CONST_INT, then the mode should
>>>>>>> come from the other object.  I think that's the fundamental problem
>>>>>>> here
>>>>>>> and that you're just papering over it by changing the caller.
>>>>>>>
>>>>>> I think my earlier explanation was a bit misleading and I may have
>>>>>> rushed into quoting the comment about both operands being const for
>>>>>> emit_store_flag_force(). The problem is with the function and I do
>>>>>> agree with your suggestion of changing the function to add the code
>>>>>> below to be a better approach than the changing the caller. I will
>>>>>> change the patch and test it.
>>>>>>
>>>>>
>>>>> This is the updated patch according to your suggestions.
>>>>>
>>>>> Testing: Checked for regressions on arm-none-linux-gnueabihf and added
>>>>> new test case.
>>>>>
>>>>> Thanks
>>>>> Sudi
>>>>>
>>>>> ChangeLog entries:
>>>>>
>>>>> *** gcc/ChangeLog ***
>>>>>
>>>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>>>
>>>>>       PR target/82096
>>>>>       * expmed.c (emit_store_flag_force): Swap if const op0
>>>>>       and change VOIDmode to mode of op0.
>>>>>
>>>>> *** gcc/testsuite/ChangeLog ***
>>>>>
>>>>> 2017-01-10  Sudakshina Das  <sudi.das@arm.com>
>>>>>
>>>>>       PR target/82096
>>>>>       * gcc.c-torture/compile/pr82096.c: New test.
>>>>
>>>> OK.
>>>
>>>
>>> Thanks. Committed as r256526.
>>> Sudi
>>>
>>
>> Could you add a guard like in other tests to skip it if the user added
>> -mfloat-abi=XXX when running the tests?
>>
>> For instance, I have a configuration where I add
>> -mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
>> and the new test fails because:
>> xgcc: error: -mfloat-abi=soft and -mfloat-abi=hard may not be used together
> It's starting to feel like the test should move into gcc.target/arm :-)
>   I nearly suggested that already.  Consider moving it into
> gcc.target/arm pre-approved along with adding the -O<whatever you need>
> to the options and whatever is needed to skip the test at the
> appropriate time.

My initial thought was also to put the test in gcc.target/arm. But I 
wanted to put it in a torture suite as this was failing at different 
optimization levels. Creating several tests for different optimization 
levels or a new torture suite just for this test did not look like the 
better options in the end and hence I put it here.

In order to fix this test, I have proposed the following patch 
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01420.html last week. I 
have added a further directive this morning just in case.

Thanks
Sudi

> 
> 
> jeff
>