C PATCH for c/65345 (file-scope _Atomic expansion with floats)

C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Marek Polacek]

Joseph reminded me that I had forgotten about this patch.  As mentioned
here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
removing the XFAILs in the tests so people are likely to see new FAILs.

I think the following targets will need similar fix as the one below:
* MIPS
* rs6000
* alpha
* sparc
* s390
* arm
* sh
* aarch64

I'm CCing the respective maintainers.  You might want to XFAIL those tests.

Applying to trunk.

2015-10-01  Marek Polacek  <polacek@redhat.com>

	PR c/65345
	* config/i386/i386.c (ix86_atomic_assign_expand_fenv): Adjust to use
	create_tmp_var_raw rather than create_tmp_var.

	* gcc.dg/atomic/pr65345-4.c: New test.
	* gcc.dg/pr65345-3.c: New test.

diff --git gcc/config/i386/i386.c gcc/config/i386/i386.c
index fe9c756..cfeba76 100644
--- gcc/config/i386/i386.c
+++ gcc/config/i386/i386.c
@@ -53128,13 +53128,13 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 {
   if (!TARGET_80387 && !TARGET_SSE_MATH)
     return;
-  tree exceptions_var = create_tmp_var (integer_type_node);
+  tree exceptions_var = create_tmp_var_raw (integer_type_node);
   if (TARGET_80387)
     {
       tree fenv_index_type = build_index_type (size_int (6));
       tree fenv_type = build_array_type (unsigned_type_node, fenv_index_type);
-      tree fenv_var = create_tmp_var (fenv_type);
-      mark_addressable (fenv_var);
+      tree fenv_var = create_tmp_var_raw (fenv_type);
+      TREE_ADDRESSABLE (fenv_var) = 1;
       tree fenv_ptr = build_pointer_type (fenv_type);
       tree fenv_addr = build1 (ADDR_EXPR, fenv_ptr, fenv_var);
       fenv_addr = fold_convert (ptr_type_node, fenv_addr);
@@ -53144,10 +53144,12 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
       tree fnclex = ix86_builtins[IX86_BUILTIN_FNCLEX];
       tree hold_fnstenv = build_call_expr (fnstenv, 1, fenv_addr);
       tree hold_fnclex = build_call_expr (fnclex, 0);
-      *hold = build2 (COMPOUND_EXPR, void_type_node, hold_fnstenv,
+      fenv_var = build4 (TARGET_EXPR, fenv_type, fenv_var, hold_fnstenv,
+			 NULL_TREE, NULL_TREE);
+      *hold = build2 (COMPOUND_EXPR, void_type_node, fenv_var,
 		      hold_fnclex);
       *clear = build_call_expr (fnclex, 0);
-      tree sw_var = create_tmp_var (short_unsigned_type_node);
+      tree sw_var = create_tmp_var_raw (short_unsigned_type_node);
       tree fnstsw_call = build_call_expr (fnstsw, 0);
       tree sw_mod = build2 (MODIFY_EXPR, short_unsigned_type_node,
 			    sw_var, fnstsw_call);
@@ -53161,8 +53163,8 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
     }
   if (TARGET_SSE_MATH)
     {
-      tree mxcsr_orig_var = create_tmp_var (unsigned_type_node);
-      tree mxcsr_mod_var = create_tmp_var (unsigned_type_node);
+      tree mxcsr_orig_var = create_tmp_var_raw (unsigned_type_node);
+      tree mxcsr_mod_var = create_tmp_var_raw (unsigned_type_node);
       tree stmxcsr = ix86_builtins[IX86_BUILTIN_STMXCSR];
       tree ldmxcsr = ix86_builtins[IX86_BUILTIN_LDMXCSR];
       tree stmxcsr_hold_call = build_call_expr (stmxcsr, 0);
diff --git gcc/testsuite/gcc.dg/atomic/pr65345-4.c gcc/testsuite/gcc.dg/atomic/pr65345-4.c
index e69de29..6d44def 100644
--- gcc/testsuite/gcc.dg/atomic/pr65345-4.c
+++ gcc/testsuite/gcc.dg/atomic/pr65345-4.c
@@ -0,0 +1,58 @@
+/* PR c/65345 */
+/* { dg-options "" } */
+
+#define CHECK(X) if (!(X)) __builtin_abort ()
+
+_Atomic float i = 5;
+_Atomic float j = 2;
+
+void
+fn1 (float a[(int) (i = 0)])
+{
+}
+
+void
+fn2 (float a[(int) (i += 2)])
+{
+}
+
+void
+fn3 (float a[(int) ++i])
+{
+}
+
+void
+fn4 (float a[(int) ++i])
+{
+}
+
+void
+fn5 (float a[(int) ++i][(int) (j = 10)])
+{
+}
+
+void
+fn6 (float a[(int) (i = 7)][(int) j--])
+{
+}
+
+int
+main ()
+{
+  float a[10];
+  float aa[10][10];
+  fn1 (a);
+  CHECK (i == 0);
+  fn2 (a);
+  CHECK (i == 2);
+  fn3 (a);
+  CHECK (i == 3);
+  fn4 (a);
+  CHECK (i == 4);
+  fn5 (aa);
+  CHECK (i == 5);
+  CHECK (j == 10);
+  fn6 (aa);
+  CHECK (i == 7);
+  CHECK (j == 9);
+}
diff --git gcc/testsuite/gcc.dg/pr65345-3.c gcc/testsuite/gcc.dg/pr65345-3.c
index e69de29..cda9364 100644
--- gcc/testsuite/gcc.dg/pr65345-3.c
+++ gcc/testsuite/gcc.dg/pr65345-3.c
@@ -0,0 +1,35 @@
+/* PR c/65345 */
+/* { dg-options "" } */
+
+_Atomic float i = 3.0f;
+
+float a1 = sizeof (i + 1.2);
+float a2 = sizeof (i = 0);
+float a3 = sizeof (i++);
+float a4 = sizeof (i--);
+float a5 = sizeof (-i);
+
+float b1 = _Alignof (i + 1);
+float b2 = _Alignof (i = 0);
+float b3 = _Alignof (i++);
+float b4 = _Alignof (i--);
+float b5 = _Alignof (-i);
+
+float c1 = i; /* { dg-error "initializer element is not constant" } */
+float c2 = (i ? 1 : 2); /* { dg-error "initializer element is not constant" } */
+float c3[(int) i]; /* { dg-error "variably modified" } */
+float c4 = 0 || i; /* { dg-error "initializer element is not constant" } */
+float c5 = (i += 10); /* { dg-error "initializer element is not constant" } */
+
+_Static_assert (_Generic (i, float: 1, default: 0) == 1, "1");
+_Static_assert (_Generic (i + 1, float: 1, default: 0) == 1, "2");
+_Static_assert (_Generic (i = 0, float: 1, default: 0) == 1, "3");
+_Static_assert (_Generic (i++, float: 1, default: 0) == 1, "4");
+_Static_assert (_Generic (i--, float: 1, default: 0) == 1, "5");
+
+_Atomic int sz = 2;
+void fn1 (float a[sz + 1]);
+void fn2 (float a[sz = 0]);
+void fn3 (float a[sz++]);
+void fn4 (float a[sz--]);
+void fn5 (float a[-sz]);

	Marek

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[David Edelsohn]

On Thu, Oct 1, 2015 at 10:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> Joseph reminded me that I had forgotten about this patch.  As mentioned
> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
> removing the XFAILs in the tests so people are likely to see new FAILs.
>
> I think the following targets will need similar fix as the one below:
> * MIPS
> * rs6000
> * alpha
> * sparc
> * s390
> * arm
> * sh
> * aarch64
>
> I'm CCing the respective maintainers.  You might want to XFAIL those tests.

Why aren't you testing the appropriate fix on all of the targets?

- David

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Marek Polacek]

On Thu, Oct 01, 2015 at 11:02:09AM -0400, David Edelsohn wrote:
> On Thu, Oct 1, 2015 at 10:49 AM, Marek Polacek <polacek@redhat.com> wrote:
> > Joseph reminded me that I had forgotten about this patch.  As mentioned
> > here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
> > removing the XFAILs in the tests so people are likely to see new FAILs.
> >
> > I think the following targets will need similar fix as the one below:
> > * MIPS
> > * rs6000
> > * alpha
> > * sparc
> > * s390
> > * arm
> > * sh
> > * aarch64
> >
> > I'm CCing the respective maintainers.  You might want to XFAIL those tests.
> 
> Why aren't you testing the appropriate fix on all of the targets?

It's very improbable that I could fix and properly test all of them;
I simply don't have the cycles and resources to fix e.g. sh/sparc/alpha/mips.

You want me to revert my fix, but I don't really see the point here; the
patch doesn't introduce any regressions, it's just that the new tests are
likely to FAIL.  It sounds preferable to me to fix 2 targets than to leave
all of them broken (and I bet many maintainers were unaware of the issue).

Would XFAILing the new tests work for you, if you don't want to see any
new FAILs?

If you still insist on reverting the patch, ok, but I think this PR is
unlikely to be resolved any time soon then.

	Marek

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Ramana Radhakrishnan]

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



On 01/10/15 17:18, Marek Polacek wrote:
> On Thu, Oct 01, 2015 at 11:02:09AM -0400, David Edelsohn wrote:
>> On Thu, Oct 1, 2015 at 10:49 AM, Marek Polacek <polacek@redhat.com> wrote:
>>> Joseph reminded me that I had forgotten about this patch.  As mentioned
>>> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
>>> removing the XFAILs in the tests so people are likely to see new FAILs.
>>>
>>> I think the following targets will need similar fix as the one below:
>>> * MIPS
>>> * rs6000
>>> * alpha
>>> * sparc
>>> * s390
>>> * arm
>>> * sh
>>> * aarch64
>>>
>>> I'm CCing the respective maintainers.  You might want to XFAIL those tests.

Thanks for the heads up. The use of the _Atomic feature
is not really target specific but more a language standards issue across all the architectures
that the GCC project supports, therefore XFAILing them is the wrong approach imho.


>>
>> Why aren't you testing the appropriate fix on all of the targets?
> 
> It's very improbable that I could fix and properly test all of them;
> I simply don't have the cycles and resources to fix e.g. sh/sparc/alpha/mips.

I don't think anyone expects you to be testing the patch on every single port .....

Even though these changes sit in the target hooks into various backends, you may be best
placed to advise how target maintainers adjust their backends. If at that point this appears to be
mechanical, it's been good practice in the community for folks to send patches
that the maintainers can fully test even if the testing has been light for the
proposed patch.

However, I am not aware of a "policy" for these things other than that these
sort of changes are selectively enforced in the community. Maybe we should think
about it ....


> 
> You want me to revert my fix, but I don't really see the point here; the
> patch doesn't introduce any regressions, it's just that the new tests are
> likely to FAIL.  It sounds preferable to me to fix 2 targets than to leave
> all of them broken (and I bet many maintainers were unaware of the issue).
> 


> Would XFAILing the new tests work for you, if you don't want to see any
> new FAILs?
> 
> If you still insist on reverting the patch, ok, but I think this PR is
> unlikely to be resolved any time soon then.
> 
> 	Marek
> 


I've had a quick look on aarch64 - changing the interface to use create_tmp_var_raw
is rather mechanical. What I'm struggling with is figuring out whether
the change for TARGET_EXPR is applicable in the arm / aarch64 backends.

It took me a couple of minutes to trial the interface changes (attached) on aarch64
as I had a cross-compiler build tree lying around and could see that the compiler
did not ICE with the 2 testcases provided and pr65345-4.c appeared to pass on hardware.


regards
Ramana


[-- Attachment #2: fenv-pr65354.txt --]
[-- Type: text/plain, Size: 1779 bytes --]

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 80916a9..716ed6e 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -1462,8 +1462,8 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
        __builtin_aarch64_set_cr (masked_cr);
        __builtin_aarch64_set_sr (masked_sr);  */
 
-  fenv_cr = create_tmp_var (unsigned_type_node);
-  fenv_sr = create_tmp_var (unsigned_type_node);
+  fenv_cr = create_tmp_var_raw (unsigned_type_node);
+  fenv_sr = create_tmp_var_raw (unsigned_type_node);
 
   get_fpcr = aarch64_builtin_decls[AARCH64_BUILTIN_GET_FPCR];
   set_fpcr = aarch64_builtin_decls[AARCH64_BUILTIN_SET_FPCR];
@@ -1476,9 +1476,9 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 			   ~(AARCH64_FE_ALL_EXCEPT));
 
   ld_fenv_cr = build2 (MODIFY_EXPR, unsigned_type_node,
-		    fenv_cr, build_call_expr (get_fpcr, 0));
+		       fenv_cr, build_call_expr (get_fpcr, 0));
   ld_fenv_sr = build2 (MODIFY_EXPR, unsigned_type_node,
-		    fenv_sr, build_call_expr (get_fpsr, 0));
+		       fenv_sr, build_call_expr (get_fpsr, 0));
 
   masked_fenv_cr = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_cr, mask_cr);
   masked_fenv_sr = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_sr, mask_sr);
@@ -1509,7 +1509,7 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 
        __atomic_feraiseexcept (new_fenv_var);  */
 
-  new_fenv_var = create_tmp_var (unsigned_type_node);
+  new_fenv_var = create_tmp_var_raw (unsigned_type_node);
   reload_fenv = build2 (MODIFY_EXPR, unsigned_type_node,
 			new_fenv_var, build_call_expr (get_fpsr, 0));
   restore_fnenv = build_call_expr (set_fpsr, 1, fenv_sr);

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[David Edelsohn]

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

On Fri, Oct 2, 2015 at 1:08 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@foss.arm.com> wrote:

>> It's very improbable that I could fix and properly test all of them;
>> I simply don't have the cycles and resources to fix e.g. sh/sparc/alpha/mips.
>
> I don't think anyone expects you to be testing the patch on every single port .....
>
> Even though these changes sit in the target hooks into various backends, you may be best
> placed to advise how target maintainers adjust their backends. If at that point this appears to be
> mechanical, it's been good practice in the community for folks to send patches
> that the maintainers can fully test even if the testing has been light for the
> proposed patch.
>
> However, I am not aware of a "policy" for these things other than that these
> sort of changes are selectively enforced in the community. Maybe we should think
> about it ....

The bug was not x86-specific.  The fix happens to be in
target-specific code, but that's the luck of the draw.  Numerous other
GCC developers have fixed bugs or added features that required tweaks
to all ports.  Not all targets are easily accessible and you certainly
can ask port maintainers to test a patch.  But writing that you don't
have the cycles to fix all of the targets is not a collegial answer.
Why do you believe that target maintainers have more cycles than you?
I didn't see you tell Uros to fix the bug on x86.  The approach may
work for your one specific bug, but it does not scale if every GCC
developer pursues the same process.

It's poor form to fix a bug only on x86 that is common to all targets
and leave the problem "as an exercise for the reader" for all other
targets -- essentially banishing the other targets to second-class
status with respect to conformance -- especially when the change is
mostly mechanical.  I don't expect developers to specifically enable
and exploit every new feature on every architecture, but had expected
bug fixes to be distributed to all targets.  "It's just not cricket."

GCC has thrived for over 25 years -- supporting a huge number of
targets and languages -- through a general sense of cooperation and
collaboration to ensure the success of the entire project.  If this is
going to degrade into a more parochial attitude, then maybe GCC will
need an explicit policy to counteract that mindset.

I am testing the attached patch for PPC.

- David

[-- Attachment #2: ZZ --]
[-- Type: application/octet-stream, Size: 1682 bytes --]

Index: rs6000.c
===================================================================
--- rs6000.c	(revision 228451)
+++ rs6000.c	(working copy)
@@ -36489,7 +36489,7 @@ rs6000_atomic_assign_expand_fenv (tree *hold, tree
 	  DECL_EXTERNAL (atomic_update_decl) = 1;
 	}
 
-      tree fenv_var = create_tmp_var (double_type_node);
+      tree fenv_var = create_tmp_var_raw (double_type_node);
       mark_addressable (fenv_var);
       tree fenv_addr = build1 (ADDR_EXPR, double_ptr_type_node, fenv_var);
 
@@ -36517,7 +36517,7 @@ rs6000_atomic_assign_expand_fenv (tree *hold, tree
   const unsigned HOST_WIDE_INT hold_exception_mask =
     HOST_WIDE_INT_C (0xffffffff00000007);
 
-  tree fenv_var = create_tmp_var (double_type_node);
+  tree fenv_var = create_tmp_var_raw (double_type_node);
 
   tree hold_mffs = build2 (MODIFY_EXPR, void_type_node, fenv_var, call_mffs);
 
@@ -36546,7 +36546,7 @@ rs6000_atomic_assign_expand_fenv (tree *hold, tree
   const unsigned HOST_WIDE_INT clear_exception_mask =
     HOST_WIDE_INT_C (0xffffffff00000000);
 
-  tree fenv_clear = create_tmp_var (double_type_node);
+  tree fenv_clear = create_tmp_var_raw (double_type_node);
 
   tree clear_mffs = build2 (MODIFY_EXPR, void_type_node, fenv_clear, call_mffs);
 
@@ -36578,7 +36578,7 @@ rs6000_atomic_assign_expand_fenv (tree *hold, tree
   const unsigned HOST_WIDE_INT new_exception_mask =
     HOST_WIDE_INT_C (0x1ff80fff);
 
-  tree old_fenv = create_tmp_var (double_type_node);
+  tree old_fenv = create_tmp_var_raw (double_type_node);
   tree update_mffs = build2 (MODIFY_EXPR, void_type_node, old_fenv, call_mffs);
 
   tree old_llu = build1 (VIEW_CONVERT_EXPR, uint64_type_node, old_fenv);

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Christophe Lyon]

On 2 October 2015 at 19:08, Ramana Radhakrishnan
<ramana.radhakrishnan@foss.arm.com> wrote:
>
>
> On 01/10/15 17:18, Marek Polacek wrote:
>> On Thu, Oct 01, 2015 at 11:02:09AM -0400, David Edelsohn wrote:
>>> On Thu, Oct 1, 2015 at 10:49 AM, Marek Polacek <polacek@redhat.com> wrote:
>>>> Joseph reminded me that I had forgotten about this patch.  As mentioned
>>>> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
>>>> removing the XFAILs in the tests so people are likely to see new FAILs.
>>>>
>>>> I think the following targets will need similar fix as the one below:
>>>> * MIPS
>>>> * rs6000
>>>> * alpha
>>>> * sparc
>>>> * s390
>>>> * arm
>>>> * sh
>>>> * aarch64
>>>>
>>>> I'm CCing the respective maintainers.  You might want to XFAIL those tests.
>
> Thanks for the heads up. The use of the _Atomic feature
> is not really target specific but more a language standards issue across all the architectures
> that the GCC project supports, therefore XFAILing them is the wrong approach imho.
>
>
>>>
>>> Why aren't you testing the appropriate fix on all of the targets?
>>
>> It's very improbable that I could fix and properly test all of them;
>> I simply don't have the cycles and resources to fix e.g. sh/sparc/alpha/mips.
>
> I don't think anyone expects you to be testing the patch on every single port .....
>
> Even though these changes sit in the target hooks into various backends, you may be best
> placed to advise how target maintainers adjust their backends. If at that point this appears to be
> mechanical, it's been good practice in the community for folks to send patches
> that the maintainers can fully test even if the testing has been light for the
> proposed patch.
>
> However, I am not aware of a "policy" for these things other than that these
> sort of changes are selectively enforced in the community. Maybe we should think
> about it ....
>
>
>>
>> You want me to revert my fix, but I don't really see the point here; the
>> patch doesn't introduce any regressions, it's just that the new tests are
>> likely to FAIL.  It sounds preferable to me to fix 2 targets than to leave
>> all of them broken (and I bet many maintainers were unaware of the issue).
>>
>
>
>> Would XFAILing the new tests work for you, if you don't want to see any
>> new FAILs?
>>
>> If you still insist on reverting the patch, ok, but I think this PR is
>> unlikely to be resolved any time soon then.
>>
>>       Marek
>>
>
>
> I've had a quick look on aarch64 - changing the interface to use create_tmp_var_raw
> is rather mechanical. What I'm struggling with is figuring out whether
> the change for TARGET_EXPR is applicable in the arm / aarch64 backends.
>
> It took me a couple of minutes to trial the interface changes (attached) on aarch64
> as I had a cross-compiler build tree lying around and could see that the compiler
> did not ICE with the 2 testcases provided and pr65345-4.c appeared to pass on hardware.
>
>
I've just created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67848
which covers aarch64 and arm-linux-gnueabihf targets.

> regards
> Ramana
>

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Marek Polacek]

On Sat, Oct 03, 2015 at 08:58:19PM -0400, David Edelsohn wrote:
> The bug was not x86-specific.  The fix happens to be in
> target-specific code, but that's the luck of the draw.  Numerous other
> GCC developers have fixed bugs or added features that required tweaks
> to all ports.  Not all targets are easily accessible and you certainly
> can ask port maintainers to test a patch.  But writing that you don't
> have the cycles to fix all of the targets is not a collegial answer.
> Why do you believe that target maintainers have more cycles than you?
> I didn't see you tell Uros to fix the bug on x86.  The approach may
> work for your one specific bug, but it does not scale if every GCC
> developer pursues the same process.

I now sort of regret that I ever took up on this PR.

One would think that bringing a bug into the open and fixing at least
one target is preferable to keeping the bug unfixed, but apparently
you think otherwise.

> It's poor form to fix a bug only on x86 that is common to all targets
> and leave the problem "as an exercise for the reader" for all other
> targets -- essentially banishing the other targets to second-class
> status with respect to conformance -- especially when the change is
> mostly mechanical.  I don't expect developers to specifically enable
> and exploit every new feature on every architecture, but had expected
> bug fixes to be distributed to all targets.  "It's just not cricket."
 
The change is not really purely mechanical; if it was, I'd provide
patches for every target.  Yes, it's same in priciple for all targets,
but it needs a bit of debugging anyway -- and I can't debug e.g. MIPS.
Also, I'm not sure if TARGET_EXPRs can be used on every target.

> GCC has thrived for over 25 years -- supporting a huge number of
> targets and languages -- through a general sense of cooperation and
> collaboration to ensure the success of the entire project.  If this is
> going to degrade into a more parochial attitude, then maybe GCC will
> need an explicit policy to counteract that mindset.
 
Again, this wasn't a regression.  If I commit some middle end change that
breaks a target, sure, I should fix it, that is clear (and I've done so
in the past).

And note that I was following Joseph's suggestion (which makes sense to me).

> I am testing the attached patch for PPC.

I'd be surprised if this worked as-is, because you also need to change (some
of) COMPOUND_EXPRs to TARGET_EXPRs, but I don't know which one in particular,
because that's not clear to me by just looking at the code.

	Marek

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Joseph Myers]

On Sat, 3 Oct 2015, David Edelsohn wrote:

> It's poor form to fix a bug only on x86 that is common to all targets
> and leave the problem "as an exercise for the reader" for all other
> targets -- essentially banishing the other targets to second-class
> status with respect to conformance -- especially when the change is
> mostly mechanical.  I don't expect developers to specifically enable
> and exploit every new feature on every architecture, but had expected
> bug fixes to be distributed to all targets.  "It's just not cricket."

I have no disagreement with that principle.  My disagreement is about how 
this particular patch fits in with such principles, where there is a 
subjective judgement involved about how separate issues with different 
targets' code are, and about how much fixing an issue for a target 
involves expertise in that architecture and back end versus expertise in 
the issue being fixed, and about the merits of checking in testcases early 
so it's as easy as possible to see whether a given target is actually 
affected.

If in such a case the judgement is that something can be more efficiently 
fixed with target expertise, it is of course important to be thorough 
about handling over the expertise in the fix (for example, in this case, 
explaining how to tell when TARGET_EXPR needs to be used).

Perhaps GCC should have an equivalent of glibc's 
<https://sourceware.org/glibc/wiki/PortStatus> to list cases where it was 
judged for a particular change that updates could most effectively be made 
by target maintainers.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Kaz Kojima]

Marek Polacek <polacek@redhat.com> wrote:
> Joseph reminded me that I had forgotten about this patch.  As mentioned
> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
> removing the XFAILs in the tests so people are likely to see new FAILs.
> 
> I think the following targets will need similar fix as the one below:
[snip]
> * sh

The attached patch is to fix ICEs for new pr65345-[45].c tests
on sh4-unknown-linux-gnu.  It's a mechanical change referring
to the original i386 patch, though I'm not sure the change
for TARGET_EXPR is really needed for SH targets.  I'd like to
apply it anyway, because it fixes the ICEs and there are no
new failures on sh4-unknown-linux-gnu.  Committed.

Regards,
	kaz
--
2015-10-05  Kaz Kojima  <kkojima@gcc.gnu.org>

	PR c/65345
	* config/sh/sh.c (sh_atomic_assign_expand_fenv): Adjust to use
	create_tmp_var_raw rather than create_tmp_var.

diff --git a/config/sh/sh.c b/config/sh/sh.c
index 904201b..c9c42ba 100644
--- a/config/sh/sh.c
+++ b/config/sh/sh.c
@@ -12109,7 +12109,7 @@ sh_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 
        __builtin_sh_set_fpscr (masked_fenv);  */
 
-  fenv_var = create_tmp_var (unsigned_type_node);
+  fenv_var = create_tmp_var_raw (unsigned_type_node);
   mask = build_int_cst (unsigned_type_node,
 			~((SH_FE_ALL_EXCEPT << SH_FE_EXCEPT_SHIFT)
 			  | SH_FE_ALL_EXCEPT));
@@ -12117,9 +12117,11 @@ sh_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 		    fenv_var, build_call_expr (sh_builtin_get_fpscr, 0));
   masked_fenv = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_var, mask);
   hold_fnclex = build_call_expr (sh_builtin_set_fpscr, 1, masked_fenv);
-  *hold = build2 (COMPOUND_EXPR, void_type_node,
-		  build2 (COMPOUND_EXPR, void_type_node, masked_fenv, ld_fenv),
-		  hold_fnclex);
+  fenv_var = build4 (TARGET_EXPR, unsigned_type_node, fenv_var,
+		     build2 (COMPOUND_EXPR, void_type_node, masked_fenv,
+			     ld_fenv),
+		     NULL_TREE, NULL_TREE);
+  *hold = build2 (COMPOUND_EXPR, void_type_node, fenv_var, hold_fnclex);
 
   /* Store the value of masked_fenv to clear the exceptions:
      __builtin_sh_set_fpscr (masked_fenv);  */
@@ -12134,7 +12136,7 @@ sh_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 
        __atomic_feraiseexcept (new_fenv_var);  */
 
-  new_fenv_var = create_tmp_var (unsigned_type_node);
+  new_fenv_var = create_tmp_var_raw (unsigned_type_node);
   reload_fenv = build2 (MODIFY_EXPR, unsigned_type_node, new_fenv_var,
 			build_call_expr (sh_builtin_get_fpscr, 0));
   restore_fnenv = build_call_expr (sh_builtin_set_fpscr, 1, fenv_var);

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Eric Botcazou]

> The attached patch is to fix ICEs for new pr65345-[45].c tests
> on sh4-unknown-linux-gnu.  It's a mechanical change referring
> to the original i386 patch, though I'm not sure the change
> for TARGET_EXPR is really needed for SH targets.

No, it is not if you don't need to make the variable addressable.  In any 
case, the failure mode is another ICE in make_decl_rtl so easy to spot.

-- 
Eric Botcazou

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Eric Botcazou]

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

> Joseph reminded me that I had forgotten about this patch.  As mentioned
> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
> removing the XFAILs in the tests so people are likely to see new FAILs.
> 
> I think the following targets will need similar fix as the one below:
> * MIPS
> * rs6000
> * alpha
> * sparc
> * s390
> * arm
> * sh
> * aarch64
> 
> I'm CCing the respective maintainers.  You might want to XFAIL those tests.

Thanks, here are the SPARC bits with an explanation for the other maintainers: 
create_tmp_var_raw must be used instead of create_tmp_var because the hook can 
be invoked outside of a function context; likewise for TREE_ADDRESSABLE vs 
mark_addressable; TARGET_EXPR is needed for variables that are addressable 
(because their address is taken) to force proper gimplification.

Tested on SPARC/Solaris, applied on the mainline.


	PR c/65345
	* config/sparc/sparc.c (sparc_atomic_assign_expand_fenv): Adjust to
	use create_tmp_var_raw rather than create_tmp_var.

-- 
Eric Botcazou

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

Index: config/sparc/sparc.c
===================================================================
--- config/sparc/sparc.c	(revision 228512)
+++ config/sparc/sparc.c	(working copy)
@@ -12540,20 +12540,23 @@ sparc_atomic_assign_expand_fenv (tree *h
 
        __builtin_load_fsr (&tmp1_var);  */
 
-  tree fenv_var = create_tmp_var (unsigned_type_node);
-  mark_addressable (fenv_var);
+  tree fenv_var = create_tmp_var_raw (unsigned_type_node);
+  TREE_ADDRESSABLE (fenv_var) = 1;
   tree fenv_addr = build_fold_addr_expr (fenv_var);
   tree stfsr = sparc_builtins[SPARC_BUILTIN_STFSR];
-  tree hold_stfsr = build_call_expr (stfsr, 1, fenv_addr);
+  tree hold_stfsr
+    = build4 (TARGET_EXPR, unsigned_type_node, fenv_var,
+	      build_call_expr (stfsr, 1, fenv_addr), NULL_TREE, NULL_TREE);
 
-  tree tmp1_var = create_tmp_var (unsigned_type_node);
-  mark_addressable (tmp1_var);
+  tree tmp1_var = create_tmp_var_raw (unsigned_type_node);
+  TREE_ADDRESSABLE (tmp1_var) = 1;
   tree masked_fenv_var
     = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_var,
 	      build_int_cst (unsigned_type_node,
 			     ~(accrued_exception_mask | trap_enable_mask)));
   tree hold_mask
-    = build2 (MODIFY_EXPR, void_type_node, tmp1_var, masked_fenv_var);
+    = build4 (TARGET_EXPR, unsigned_type_node, tmp1_var, masked_fenv_var,
+	      NULL_TREE, NULL_TREE);
 
   tree tmp1_addr = build_fold_addr_expr (tmp1_var);
   tree ldfsr = sparc_builtins[SPARC_BUILTIN_LDFSR];
@@ -12578,10 +12581,12 @@ sparc_atomic_assign_expand_fenv (tree *h
          tmp2_var >>= 5;
        __atomic_feraiseexcept ((int) tmp2_var);  */
 
-  tree tmp2_var = create_tmp_var (unsigned_type_node);
-  mark_addressable (tmp2_var);
-  tree tmp3_addr = build_fold_addr_expr (tmp2_var);
-  tree update_stfsr = build_call_expr (stfsr, 1, tmp3_addr);
+  tree tmp2_var = create_tmp_var_raw (unsigned_type_node);
+  TREE_ADDRESSABLE (tmp2_var) = 1;
+  tree tmp2_addr = build_fold_addr_expr (tmp2_var);
+  tree update_stfsr
+    = build4 (TARGET_EXPR, unsigned_type_node, tmp2_var,
+	      build_call_expr (stfsr, 1, tmp2_addr), NULL_TREE, NULL_TREE);
 
   tree update_ldfsr = build_call_expr (ldfsr, 1, fenv_addr);
 

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Kaz Kojima]

Eric Botcazou <ebotcazou@adacore.com> wrote:
> No, it is not if you don't need to make the variable addressable.  In any 
> case, the failure mode is another ICE in make_decl_rtl so easy to spot.

Thanks for your explanation!  It clarifies the intent of
the original i386 patch.

Regards,
	kaz

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Ramana Radhakrishnan]

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



On 06/10/15 10:12, Eric Botcazou wrote:
>> Joseph reminded me that I had forgotten about this patch.  As mentioned
>> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
>> removing the XFAILs in the tests so people are likely to see new FAILs.
>>
>> I think the following targets will need similar fix as the one below:
>> * MIPS
>> * rs6000
>> * alpha
>> * sparc
>> * s390
>> * arm
>> * sh
>> * aarch64
>>
>> I'm CCing the respective maintainers.  You might want to XFAIL those tests.
> 
> Thanks, here are the SPARC bits with an explanation for the other maintainers: 
> create_tmp_var_raw must be used instead of create_tmp_var because the hook can 
> be invoked outside of a function context; likewise for TREE_ADDRESSABLE vs 
> mark_addressable; TARGET_EXPR is needed for variables that are addressable 
> (because their address is taken) to force proper gimplification.
> 
> Tested on SPARC/Solaris, applied on the mainline.
> 
> 
> 	PR c/65345
> 	* config/sparc/sparc.c (sparc_atomic_assign_expand_fenv): Adjust to
> 	use create_tmp_var_raw rather than create_tmp_var.
> 


Thanks for the explanation Eric, by that explanation I do not see the need to adjust for TARGET_EXPR or mark_addressable in the backends.

Here are the patches that I'm testing - I will apply the ARM one after testing finishes - my previous testing broke because of some other reasons.

The AArch64 patch cleared testing - ok to apply ?

regards
Ramana


PR c/65345

	* config/arm/arm-builtins.c (arm_atomic_assign_expand_fenv): Use create_tmp_var_raw instead of create_tmp_var.


PR c/65345

	* config/aarch64/aarch64-builtins.c (aarch64_atomic_assign_expand_fenv): Use create_tmp_var_raw instead of create_tmp_var.

[-- Attachment #2: fenv-arm-pr65354.txt --]
[-- Type: text/plain, Size: 1199 bytes --]

commit 8543c3f0c83059c89dc1e09716dab27e3414d69d
Author: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Date:   Sat Oct 3 00:09:55 2015 +0100

    Fix create_tmp_var_raw ARM

diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
index 0f5a1f1..17cf8e7 100644
--- a/gcc/config/arm/arm-builtins.c
+++ b/gcc/config/arm/arm-builtins.c
@@ -2974,7 +2974,7 @@ arm_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 
        __builtin_arm_set_fpscr (masked_fenv);  */
 
-  fenv_var = create_tmp_var (unsigned_type_node);
+  fenv_var = create_tmp_var_raw (unsigned_type_node);
   get_fpscr = arm_builtin_decls[ARM_BUILTIN_GET_FPSCR];
   set_fpscr = arm_builtin_decls[ARM_BUILTIN_SET_FPSCR];
   mask = build_int_cst (unsigned_type_node,
@@ -3001,7 +3001,7 @@ arm_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 
        __atomic_feraiseexcept (new_fenv_var);  */
 
-  new_fenv_var = create_tmp_var (unsigned_type_node);
+  new_fenv_var = create_tmp_var_raw (unsigned_type_node);
   reload_fenv = build2 (MODIFY_EXPR, unsigned_type_node, new_fenv_var,
 			build_call_expr (get_fpscr, 0));
   restore_fnenv = build_call_expr (set_fpscr, 1, fenv_var);

[-- Attachment #3: fenv-aarch64-pr65354.txt --]
[-- Type: text/plain, Size: 1779 bytes --]

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index 80916a9..716ed6e 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -1462,8 +1462,8 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
        __builtin_aarch64_set_cr (masked_cr);
        __builtin_aarch64_set_sr (masked_sr);  */
 
-  fenv_cr = create_tmp_var (unsigned_type_node);
-  fenv_sr = create_tmp_var (unsigned_type_node);
+  fenv_cr = create_tmp_var_raw (unsigned_type_node);
+  fenv_sr = create_tmp_var_raw (unsigned_type_node);
 
   get_fpcr = aarch64_builtin_decls[AARCH64_BUILTIN_GET_FPCR];
   set_fpcr = aarch64_builtin_decls[AARCH64_BUILTIN_SET_FPCR];
@@ -1476,9 +1476,9 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 			   ~(AARCH64_FE_ALL_EXCEPT));
 
   ld_fenv_cr = build2 (MODIFY_EXPR, unsigned_type_node,
-		    fenv_cr, build_call_expr (get_fpcr, 0));
+		       fenv_cr, build_call_expr (get_fpcr, 0));
   ld_fenv_sr = build2 (MODIFY_EXPR, unsigned_type_node,
-		    fenv_sr, build_call_expr (get_fpsr, 0));
+		       fenv_sr, build_call_expr (get_fpsr, 0));
 
   masked_fenv_cr = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_cr, mask_cr);
   masked_fenv_sr = build2 (BIT_AND_EXPR, unsigned_type_node, fenv_sr, mask_sr);
@@ -1509,7 +1509,7 @@ aarch64_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 
        __atomic_feraiseexcept (new_fenv_var);  */
 
-  new_fenv_var = create_tmp_var (unsigned_type_node);
+  new_fenv_var = create_tmp_var_raw (unsigned_type_node);
   reload_fenv = build2 (MODIFY_EXPR, unsigned_type_node,
 			new_fenv_var, build_call_expr (get_fpsr, 0));
   restore_fnenv = build_call_expr (set_fpsr, 1, fenv_sr);

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Marcus Shawcroft]

On 6 October 2015 at 12:29, Ramana Radhakrishnan
<ramana.radhakrishnan@foss.arm.com> wrote:

> Thanks for the explanation Eric, by that explanation I do not see the need to adjust for TARGET_EXPR or mark_addressable in the backends.
>
> Here are the patches that I'm testing - I will apply the ARM one after testing finishes - my previous testing broke because of some other reasons.
>
> The AArch64 patch cleared testing - ok to apply ?
>

> PR c/65345
>
>         * config/aarch64/aarch64-builtins.c (aarch64_atomic_assign_expand_fenv): Use create_tmp_var_raw instead of create_tmp_var.

OK /Marcus

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Uros Bizjak]

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

On Thu, Oct 1, 2015 at 4:49 PM, Marek Polacek <polacek@redhat.com> wrote:
> Joseph reminded me that I had forgotten about this patch.  As mentioned
> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
> removing the XFAILs in the tests so people are likely to see new FAILs.

2015-10-06  Uros Bizjak  <ubizjak@gmail.com>

    PR c/65345
    * config/alpha/alpha.c (alpha_atomic_assign_expand_fenv): Use
    create_tmp_var_raw instead of create_tmp_var.

Tested with alpha-linux-gnu crosscompiler on the attached testcase,
since native alpha bootstrap currently fails due to PR 67766.

Uros.

[-- Attachment #2: a.diff.txt --]
[-- Type: text/plain, Size: 973 bytes --]

Index: config/alpha/alpha.c
===================================================================
--- config/alpha/alpha.c	(revision 228525)
+++ config/alpha/alpha.c	(working copy)
@@ -9765,7 +9765,7 @@ alpha_atomic_assign_expand_fenv (tree *hold, tree
 
        __ieee_set_fp_control (masked_fenv);  */
 
-  fenv_var = create_tmp_var (long_unsigned_type_node);
+  fenv_var = create_tmp_var_raw (long_unsigned_type_node);
   get_fpscr
     = build_fn_decl ("__ieee_get_fp_control",
 		     build_function_type_list (long_unsigned_type_node, NULL));
@@ -9794,7 +9794,7 @@ alpha_atomic_assign_expand_fenv (tree *hold, tree
 
        __atomic_feraiseexcept (new_fenv_var);  */
 
-  new_fenv_var = create_tmp_var (long_unsigned_type_node);
+  new_fenv_var = create_tmp_var_raw (long_unsigned_type_node);
   reload_fenv = build2 (MODIFY_EXPR, long_unsigned_type_node, new_fenv_var,
 			build_call_expr (get_fpscr, 0));
   restore_fnenv = build_call_expr (set_fpscr, 1, fenv_var);

Re: C PATCH for c/65345 (file-scope _Atomic expansion with floats)

[Andreas Krebbel]

On Thu, Oct 01, 2015 at 04:49:41PM +0200, Marek Polacek wrote:
> Joseph reminded me that I had forgotten about this patch.  As mentioned
> here <https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01792.html>, I'm
> removing the XFAILs in the tests so people are likely to see new FAILs.
> 
> I think the following targets will need similar fix as the one below:
> * MIPS
> * rs6000
> * alpha
> * sparc
> * s390
> * arm
> * sh
> * aarch64
> 
> I'm CCing the respective maintainers.  You might want to XFAIL those tests.

I've applied the following patch for s390.

Bye,

-Andreas-

gcc/ChangeLog:

2015-10-08  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>

	PR c/65345
	* config/s390/s390.c (s390_atomic_assign_expand_fenv): Use
	create_tmp_var_raw instead of create_tmp_var.

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 5ab6ce7..b994cd2 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -13678,7 +13678,7 @@ s390_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
   tree sfpc = s390_builtin_decls[S390_BUILTIN_s390_sfpc];
   tree efpc = s390_builtin_decls[S390_BUILTIN_s390_efpc];
   tree call_efpc = build_call_expr (efpc, 0);
-  tree fenv_var = create_tmp_var (unsigned_type_node);
+  tree fenv_var = create_tmp_var_raw (unsigned_type_node);
 
 #define FPC_EXCEPTION_MASK	 HOST_WIDE_INT_UC (0xf8000000)
 #define FPC_FLAGS_MASK		 HOST_WIDE_INT_UC (0x00f80000)
@@ -13714,7 +13714,7 @@ s390_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
   __builtin_s390_sfpc (fenv_var);
   __atomic_feraiseexcept ((old_fpc & FPC_FLAGS_MASK) >> FPC_FLAGS_SHIFT);  */
 
-  old_fpc = create_tmp_var (unsigned_type_node);
+  old_fpc = create_tmp_var_raw (unsigned_type_node);
   tree store_old_fpc = build2 (MODIFY_EXPR, void_type_node,
 			       old_fpc, call_efpc);