[PATCH] cfgrtl: Fix up fixup_partitions caused ICE [PR99085]

[PATCH] cfgrtl: Fix up fixup_partitions caused ICE [PR99085]

[Jakub Jelinek]

Hi!

fixup_partitions sometimes changes some basic blocks from hot partition to
cold partition, in particular if after unreachable block removal or other
optimizations a hot partition block is dominated by cold partition block(s).
It fixes up the edges and jumps on those edges, but when after reorder
blocks and in rtl (non-cfglayout) mode that is clearly not enough, because
it keeps the block order the same and so we can end up with more than
1 hot/cold section transition in the same function.

So, this patch fixes that up too.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2021-02-15  Jakub Jelinek  <jakub@redhat.com>

	PR target/99085
	* cfgrtl.c (fixup_partitions): When changing some bbs from hot to cold
	partitions, if in non-layout mode after reorder_blocks also move
	affected blocks to ensure a single partition transition.

	* gcc.dg/graphite/pr99085.c: New test.

--- gcc/cfgrtl.c.jj	2021-02-10 19:27:33.000000000 +0100
+++ gcc/cfgrtl.c	2021-02-15 16:47:41.625798717 +0100
@@ -2414,8 +2414,6 @@ find_partition_fixes (bool flag_only)
 void
 fixup_partitions (void)
 {
-  basic_block bb;
-
   if (!crtl->has_bb_partition)
     return;
 
@@ -2436,10 +2434,61 @@ fixup_partitions (void)
   /* Do the partition fixup after all necessary blocks have been converted to
      cold, so that we only update the region crossings the minimum number of
      places, which can require forcing edges to be non fallthru.  */
-  while (! bbs_to_fix.is_empty ())
+  if (! bbs_to_fix.is_empty ())
     {
-      bb = bbs_to_fix.pop ();
-      fixup_new_cold_bb (bb);
+      do
+	{
+	  basic_block bb = bbs_to_fix.pop ();
+	  fixup_new_cold_bb (bb);
+	}
+      while (! bbs_to_fix.is_empty ());
+
+      /* Fix up hot cold block grouping if needed.  */
+      if (crtl->bb_reorder_complete && current_ir_type () == IR_RTL_CFGRTL)
+	{
+	  basic_block bb, first = NULL, second = NULL;
+	  int current_partition = BB_UNPARTITIONED;
+
+	  FOR_EACH_BB_FN (bb, cfun)
+	    {
+	      if (current_partition != BB_UNPARTITIONED
+		  && BB_PARTITION (bb) != current_partition)
+		{
+		  if (first == NULL)
+		    first = bb;
+		  else if (second == NULL)
+		    second = bb;
+		  else
+		    {
+		      /* If we switch partitions for the 3rd, 5th etc. time,
+			 move bbs first (inclusive) .. second (exclusive) right
+			 before bb.  */
+		      basic_block prev_first = first->prev_bb;
+		      basic_block prev_second = second->prev_bb;
+		      basic_block prev_bb = bb->prev_bb;
+		      prev_first->next_bb = second;
+		      second->prev_bb = prev_first;
+		      prev_second->next_bb = bb;
+		      bb->prev_bb = prev_second;
+		      prev_bb->next_bb = first;
+		      first->prev_bb = prev_bb;
+		      rtx_insn *prev_first_insn = PREV_INSN (BB_HEAD (first));
+		      rtx_insn *prev_second_insn
+			= PREV_INSN (BB_HEAD (second));
+		      rtx_insn *prev_bb_insn = PREV_INSN (BB_HEAD (bb));
+		      SET_NEXT_INSN (prev_first_insn) = BB_HEAD (second);
+		      SET_PREV_INSN (BB_HEAD (second)) = prev_first_insn;
+		      SET_NEXT_INSN (prev_second_insn) = BB_HEAD (bb);
+		      SET_PREV_INSN (BB_HEAD (bb)) = prev_second_insn;
+		      SET_NEXT_INSN (prev_bb_insn) = BB_HEAD (first);
+		      SET_PREV_INSN (BB_HEAD (first)) = prev_bb_insn;
+		      second = NULL;
+		    }
+		}
+	      current_partition = BB_PARTITION (bb);
+	    }
+	  gcc_assert (!second);
+	}
     }
 }
 
--- gcc/testsuite/gcc.dg/graphite/pr99085.c.jj	2021-02-15 16:52:59.313169888 +0100
+++ gcc/testsuite/gcc.dg/graphite/pr99085.c	2021-02-15 16:51:54.589910609 +0100
@@ -0,0 +1,20 @@
+/* PR target/99085 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fgraphite-identity -fsel-sched-pipelining -fselective-scheduling2" } */
+
+void
+foo (int m, int n, int o, int i)
+{
+  long double a2[m];
+  int c2[n][o];
+  int j, k;
+
+  while (i < m)
+    a2[i++] = 13.132L;
+
+  for (j = 0; j < n; j++)
+    for (k = 0; k < o; k++)
+      c2[j][k] = 1;
+
+  __builtin_abort ();
+}

	Jakub

Patch ping

[Jakub Jelinek]

Hi!

I'd like to ping the
https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565350.html
patch, P2 PR99085 ice-on-valid-code fix in fixup_partitions.

Thanks

	Jakub

Patch ping^2

[Jakub Jelinek]

On Tue, Feb 23, 2021 at 09:49:08AM +0100, Jakub Jelinek via Gcc-patches wrote:
> I'd like to ping the
> https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565350.html
> patch, P2 PR99085 ice-on-valid-code fix in fixup_partitions.

Ping

Thanks

	Jakub

Re: [PATCH] cfgrtl: Fix up fixup_partitions caused ICE [PR99085]

[Jeff Law]

On 2/16/21 1:13 AM, Jakub Jelinek via Gcc-patches wrote:
> Hi!
>
> fixup_partitions sometimes changes some basic blocks from hot partition to
> cold partition, in particular if after unreachable block removal or other
> optimizations a hot partition block is dominated by cold partition block(s).
> It fixes up the edges and jumps on those edges, but when after reorder
> blocks and in rtl (non-cfglayout) mode that is clearly not enough, because
> it keeps the block order the same and so we can end up with more than
> 1 hot/cold section transition in the same function.
>
> So, this patch fixes that up too.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2021-02-15  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/99085
> 	* cfgrtl.c (fixup_partitions): When changing some bbs from hot to cold
> 	partitions, if in non-layout mode after reorder_blocks also move
> 	affected blocks to ensure a single partition transition.
>
> 	* gcc.dg/graphite/pr99085.c: New test.
OK
jeff

Re: Patch ping^2

[Jan Hubicka]

> Hi!
> 
> I'd like to ping 2 patches:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644580.html                                                                                                                    
> PR113617 P1 - Handle private COMDAT function symbol reference in readonly data section                                                                                                
>                                                                                                                                                                                       
> More details in the                                                                                                                                                                   
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/thread.html#644121                                                                                                             
> and                                                                                                                                                                                   
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/thread.html#644486                                                                                                             
> threads.                                                                                                                                                                              

I went through the thread - it looks like an issue that was latent for
quite some time.  I think it belongs to cgraph/symtab maintenance, so I
think I can OK this patch.

Honza
> 
> and
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645326.html
> i386: Enable _BitInt support on ia32
> 
> all the FAILs mentioned in that mail have been fixed by now.
> 
> Thanks
> 
> 	Jakub
> 

Re: Patch ping^2

[Uros Bizjak]

On Mon, Feb 26, 2024 at 10:33 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> I'd like to ping 2 patches:

> https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645326.html
> i386: Enable _BitInt support on ia32
>
> all the FAILs mentioned in that mail have been fixed by now.

LGTM, based on HJ's advice.

Uros.

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping 2 patches:

https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644580.html                                                                                                                    
PR113617 P1 - Handle private COMDAT function symbol reference in readonly data section                                                                                                
                                                                                                                                                                                      
More details in the                                                                                                                                                                   
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/thread.html#644121                                                                                                             
and                                                                                                                                                                                   
https://gcc.gnu.org/pipermail/gcc-patches/2024-January/thread.html#644486                                                                                                             
threads.                                                                                                                                                                              

and

https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645326.html
i386: Enable _BitInt support on ia32

all the FAILs mentioned in that mail have been fixed by now.

Thanks

	Jakub

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping following patch:

> https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566669.html
> PR99490 dwarf2out -gsplit-dwarf ranges fixes

Thanks

	Jakub

Re: Patch ping^2

[Richard Biener]

On Tue, 17 Mar 2020, Jakub Jelinek wrote:

> Hi!
> 
> I'd like to ping this patch again:

OK.

Thanks,
Richard.

> On Tue, Mar 10, 2020 at 01:28:19PM +0100, Jakub Jelinek wrote:
> > I'd like to ping the
> > https://gcc.gnu.org/legacy-ml/gcc-patches/2020-03/msg00154.html
> >   P1 PR94015
> > patch, with the https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94015#c5
> > testcase instead of the one sent in the patch, so that it FAILs without the
> > fix on more targets and more reliably.

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping this patch again:

On Tue, Mar 10, 2020 at 01:28:19PM +0100, Jakub Jelinek wrote:
> I'd like to ping the
> https://gcc.gnu.org/legacy-ml/gcc-patches/2020-03/msg00154.html
>   P1 PR94015
> patch, with the https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94015#c5
> testcase instead of the one sent in the patch, so that it FAILs without the
> fix on more targets and more reliably.

	Jakub

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping following patches:

- PR85480 improve AVX512 128-bit insertion into 512-bit zero vector
  http://gcc.gnu.org/ml/gcc-patches/2018-04/msg01058.html

- PR85572 implement absv2di2 and absv4di2 expanders for pre-avx512vl
  http://gcc.gnu.org/ml/gcc-patches/2018-04/msg01341.html

Thanks

	Jakub

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping the

http://gcc.gnu.org/ml/gcc-patches/2018-03/msg01244.html
  - PR83157 - improve debug info for x86 setcc peepholes

patch.  Thanks.

	Jakub

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping a bunch of patches:

http://gcc.gnu.org/ml/gcc-patches/2017-11/msg02521.html                                                                                            
  PR c++/83205 - diagnose invalid std::tuple_size<X>::value for structured                                                                         
                 bindings; the follow-up with plural spelling is approved                                                                          
                 already                                                                                                                           
                                                                                                                                                   
http://gcc.gnu.org/ml/gcc-patches/2017-11/msg02629.html                                                                                            
  PR c++/83217 - handle references to non-complete type in structured                                                                              
                 bindings                                                                                                                          

http://gcc.gnu.org/ml/gcc-patches/2017-12/msg00184.html
  PR c++/81197 - fix ICE with structured bindings and extend_ref_init_temps

http://gcc.gnu.org/ml/gcc-patches/2017-12/msg00391.html
  PR c++/83300 - RFC fix for C++ late attribute handling

http://gcc.gnu.org/ml/gcc-patches/2017-12/msg00507.html
  PR c++/80135, c++/81922 - fix ICEs with flexible array member initialization
			    in nested contexts

http://gcc.gnu.org/ml/gcc-patches/2017-12/msg00395.html
  PR sanitizer/81281 - further improvements for match.pd
		       (T)(P + A) - (T)(P + B) -> (T)A - (T)B optimization

http://gcc.gnu.org/ml/gcc-patches/2017-12/msg00346.html
  PR target/41455, target/82935 - use tail calls in aggregate copy or clear
				  expanded as memcpy or memset call if possible

	Jakub

Re: Patch ping^2

[Jim Wilson]

On 11/14/2017 08:29 AM, Jakub Jelinek wrote:
> On Mon, Nov 06, 2017 at 05:22:36PM +0100, Jakub Jelinek wrote:
>> I'd like to ping the:
>>
>>    http://gcc.gnu.org/ml/gcc-patches/2017-10/msg01895.html
>>    PR debug/82718
>>    Fix DWARF5 .debug_loclist handling with hot/cold partitioning
>>
>> patch.  Thanks
> 
> Ping^2.

The testcase doesn't fail on mainline anymore.  It also doesn't fail 
with the 2017-11-05 snapshot.  It does fail with the 2017-10-22 
snapshot.  Perhaps something subtle changed in the optimizer.  But at 
the moment the testcase isn't doing anything useful.  Maybe it can be fixed?

The dwarf2out.c patch looks OK to me though.

Jim

Patch ping^2

[Jakub Jelinek]

On Mon, Nov 06, 2017 at 05:22:36PM +0100, Jakub Jelinek wrote:
> I'd like to ping the:
> 
>   http://gcc.gnu.org/ml/gcc-patches/2017-10/msg01895.html
>   PR debug/82718
>   Fix DWARF5 .debug_loclist handling with hot/cold partitioning
> 
> patch.  Thanks

Ping^2.

	Jakub

Re: Patch ping^2

[Jeff Law]

On 04/18/2017 07:55 AM, Jakub Jelinek wrote:
> I'd like to ping following patch:
> 
> PR debug/80263
>     http://gcc.gnu.org/ml/gcc-patches/2017-04/msg00004.html
>     avoid emitting sizetype artificial name into debug info
OK.
jeff

Patch ping^2

[Jakub Jelinek]

I'd like to ping following patch:

PR debug/80263
   http://gcc.gnu.org/ml/gcc-patches/2017-04/msg00004.html
   avoid emitting sizetype artificial name into debug info

Thanks

	Jakub

Re: Patch ping^2

[Jakub Jelinek]

On Tue, Feb 14, 2017 at 10:05:04AM -0500, Nathan Sidwell wrote:
> On 02/13/2017 10:46 AM, Jakub Jelinek wrote:
> > Hi!
> > 
> > I'd like to ping a couple of patches:
> 
> > - C++ P1 PR79288 - wrong default TLS model for __thread static data members
> >   http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02349.html
> 
> This is ok, but don't you think the changelog is misleading?  In your
> description you say it needs DECL_EXTERNAL set, but the changelog says
> 'inline', which isn't something static member vars have (although I can see
> how it's involved in DECL_EXTERNAL setting).

It is something static member vars can have (explicitly or implicitly).

In 6.x we had:
                /* Even if there is an in-class initialization, DECL
                   is considered undefined until an out-of-class
                   definition is provided.  */
                DECL_EXTERNAL (decl) = 1;

                if (thread_p)
                  {
                    CP_DECL_THREAD_LOCAL_P (decl) = true;
                    if (!processing_template_decl)
                      set_decl_tls_model (decl, decl_default_tls_model (decl));
                    if (declspecs->gnu_thread_keyword_p)
                      SET_DECL_GNU_TLS_P (decl);
                  }
...
With the addition of C++17 inline vars I changed that to:
                if (thread_p)
                  {
                    CP_DECL_THREAD_LOCAL_P (decl) = true;
                    if (!processing_template_decl)
                      set_decl_tls_model (decl, decl_default_tls_model (decl));
                    if (declspecs->gnu_thread_keyword_p)
                      SET_DECL_GNU_TLS_P (decl);
                  }
...

                if (inlinep)
                  mark_inline_variable (decl);

                if (!DECL_VAR_DECLARED_INLINE_P (decl)
                    && !(cxx_dialect >= cxx1z && constexpr_p))
                  /* Even if there is an in-class initialization, DECL
                     is considered undefined until an out-of-class
                     definition is provided, unless this is an inline
                     variable.  */
                  DECL_EXTERNAL (decl) = 1;
because inline static data members, explicit or implicit, really shouldn't
be marked DECL_EXTERNAL, they have in-class definition rather than a mere
declaration.
The patch just changes that back to:
...
                if (inlinep)
                  mark_inline_variable (decl);

                if (!DECL_VAR_DECLARED_INLINE_P (decl)
                    && !(cxx_dialect >= cxx1z && constexpr_p))
                  /* Even if there is an in-class initialization, DECL
                     is considered undefined until an out-of-class
                     definition is provided, unless this is an inline
                     variable.  */
                  DECL_EXTERNAL (decl) = 1;

                if (thread_p)
                  {
                    CP_DECL_THREAD_LOCAL_P (decl) = true;
                    if (!processing_template_decl)
                      set_decl_tls_model (decl, decl_default_tls_model (decl));
                    if (declspecs->gnu_thread_keyword_p)
                      SET_DECL_GNU_TLS_P (decl);
                  }

	Jakub

Re: Patch ping^2

[Nathan Sidwell]

On 02/13/2017 10:46 AM, Jakub Jelinek wrote:
> Hi!
>
> I'd like to ping a couple of patches:

> - C++ P1 PR79288 - wrong default TLS model for __thread static data members
>   http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02349.html

This is ok, but don't you think the changelog is misleading?  In your 
description you say it needs DECL_EXTERNAL set, but the changelog says 
'inline', which isn't something static member vars have (although I can 
see how it's involved in DECL_EXTERNAL setting).

nathan
-- 
Nathan Sidwell

Re: Patch ping^2

[Nathan Sidwell]

On 02/13/2017 12:28 PM, Jakub Jelinek wrote:

>
> The reason for that VOID_TYPE_P stuff in COND_EXPR handling is not
> that stabilize_expr doesn't like it, but to avoid warning twice.
> If I replace all 3 VOID_TYPE_P tests (with the patch) in
> cp_build_modify_expr with 0 && VOID_TYPE_P, the above testcase
> errors 6 times instead of 4 times as it should.
> For the case when lhs is a pre-inc/decrement or modify_expr, we don't have
> this problem, rhs is used just one in cp_convert_for_assignment where this
> error is diagnosed normally.
>
> Thus, I'll bootstrap/regtest following simplified version of the patch
> instead:


ok, thanks

nathan

-- 
Nathan Sidwell

Re: Patch ping^2

[Jakub Jelinek]

On Mon, Feb 13, 2017 at 05:44:54PM +0100, Jakub Jelinek wrote:
> On Mon, Feb 13, 2017 at 11:41:48AM -0500, Nathan Sidwell wrote:
> > On 02/13/2017 10:46 AM, Jakub Jelinek wrote:
> > > Hi!
> > > 
> > > I'd like to ping a couple of patches:
> > > 
> > > - C++ P1 PR79232 - ICEs and wrong-code with COMPOUND_EXPR on lhs of assignment
> > >   http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02341.html
> > 
> > What puzzles me about (and may be an existing orthogonal issue), is the
> > checking for TYPE(rhs) == VOID.  In the current code it's only explicitly
> > checked in the lhs == COND_EXPR case, which is strange.  Why isn't it a
> > general constraint?
> 
> I'll double check; copied this from the COND_EXPR case which had the same.
> I believe the reason is that this is (or ought to be) checked later on,
> but stabilitize_expr wouldn't work well if it is called with a void
> expression.

Ok, so tried
void bar (int);

int
foo (int x, int *y, int z, int w, int u)
{
  y[++z] = bar (++u);
  (x++, y[++z]) = bar (++u);
  (w ? y[++z] : y[z++]) = bar (++u);
  (x++, (w ? y[++z] : y[z++])) = bar (++u);
  return x + z + u;
}

The reason for that VOID_TYPE_P stuff in COND_EXPR handling is not
that stabilize_expr doesn't like it, but to avoid warning twice.
If I replace all 3 VOID_TYPE_P tests (with the patch) in
cp_build_modify_expr with 0 && VOID_TYPE_P, the above testcase
errors 6 times instead of 4 times as it should.
For the case when lhs is a pre-inc/decrement or modify_expr, we don't have
this problem, rhs is used just one in cp_convert_for_assignment where this
error is diagnosed normally.

Thus, I'll bootstrap/regtest following simplified version of the patch
instead:

2017-02-13  Jakub Jelinek  <jakub@redhat.com>

	PR c++/79232
	* typeck.c (cp_build_modify_expr): Handle properly COMPOUND_EXPRs
	on lhs that have {PRE{DEC,INC}REMENT,MODIFY,MIN,MAX,COND}_EXPR
	in the rightmost operand.

	* g++.dg/cpp1z/eval-order4.C: New test.
	* g++.dg/other/pr79232.C: New test.

--- gcc/cp/typeck.c.jj	2017-01-31 22:36:33.762297835 +0100
+++ gcc/cp/typeck.c	2017-02-13 18:22:06.927396122 +0100
@@ -7571,16 +7571,26 @@ tree
 cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode,
 		      tree rhs, tsubst_flags_t complain)
 {
-  tree result;
+  tree result = NULL_TREE;
   tree newrhs = rhs;
   tree lhstype = TREE_TYPE (lhs);
+  tree olhs = lhs;
   tree olhstype = lhstype;
   bool plain_assign = (modifycode == NOP_EXPR);
+  bool compound_side_effects_p = false;
+  tree preeval = NULL_TREE;
 
   /* Avoid duplicate error messages from operands that had errors.  */
   if (error_operand_p (lhs) || error_operand_p (rhs))
     return error_mark_node;
 
+  while (TREE_CODE (lhs) == COMPOUND_EXPR)
+    {
+      if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
+	compound_side_effects_p = true;
+      lhs = TREE_OPERAND (lhs, 1);
+    }
+
   /* Handle control structure constructs used as "lvalues".  Note that we
      leave COMPOUND_EXPR on the LHS because it is sequenced after the RHS.  */
   switch (TREE_CODE (lhs))
@@ -7588,20 +7598,41 @@ cp_build_modify_expr (location_t loc, tr
       /* Handle --foo = 5; as these are valid constructs in C++.  */
     case PREDECREMENT_EXPR:
     case PREINCREMENT_EXPR:
+      if (compound_side_effects_p)
+	newrhs = rhs = stabilize_expr (rhs, &preeval);
       if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
 	lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
 		      cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
 		      TREE_OPERAND (lhs, 1));
       lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
+    maybe_add_compound:
+      /* If we had (bar, --foo) = 5; or (bar, (baz, --foo)) = 5;
+	 and looked through the COMPOUND_EXPRs, readd them now around
+	 the resulting lhs.  */
+      if (TREE_CODE (olhs) == COMPOUND_EXPR)
+	{
+	  lhs = build2 (COMPOUND_EXPR, lhstype, TREE_OPERAND (olhs, 0), lhs);
+	  tree *ptr = &TREE_OPERAND (lhs, 1);
+	  for (olhs = TREE_OPERAND (olhs, 1);
+	       TREE_CODE (olhs) == COMPOUND_EXPR;
+	       olhs = TREE_OPERAND (olhs, 1))
+	    {
+	      *ptr = build2 (COMPOUND_EXPR, lhstype,
+			     TREE_OPERAND (olhs, 0), *ptr);
+	      ptr = &TREE_OPERAND (*ptr, 1);
+	    }
+	}
       break;
 
     case MODIFY_EXPR:
+      if (compound_side_effects_p)
+	newrhs = rhs = stabilize_expr (rhs, &preeval);
       if (TREE_SIDE_EFFECTS (TREE_OPERAND (lhs, 0)))
 	lhs = build2 (TREE_CODE (lhs), TREE_TYPE (lhs),
 		      cp_stabilize_reference (TREE_OPERAND (lhs, 0)),
 		      TREE_OPERAND (lhs, 1));
       lhs = build2 (COMPOUND_EXPR, lhstype, lhs, TREE_OPERAND (lhs, 0));
-      break;
+      goto maybe_add_compound;
 
     case MIN_EXPR:
     case MAX_EXPR:
@@ -7629,7 +7660,6 @@ cp_build_modify_expr (location_t loc, tr
 	   except that the RHS goes through a save-expr
 	   so the code to compute it is only emitted once.  */
 	tree cond;
-	tree preeval = NULL_TREE;
 
 	if (VOID_TYPE_P (TREE_TYPE (rhs)))
 	  {
@@ -7655,14 +7685,31 @@ cp_build_modify_expr (location_t loc, tr
 
 	if (cond == error_mark_node)
 	  return cond;
+	/* If we had (e, (a ? b : c)) = d; or (e, (f, (a ? b : c))) = d;
+	   and looked through the COMPOUND_EXPRs, readd them now around
+	   the resulting cond before adding the preevaluated rhs.  */
+	if (TREE_CODE (olhs) == COMPOUND_EXPR)
+	  {
+	    cond = build2 (COMPOUND_EXPR, TREE_TYPE (cond),
+			   TREE_OPERAND (olhs, 0), cond);
+	    tree *ptr = &TREE_OPERAND (cond, 1);
+	    for (olhs = TREE_OPERAND (olhs, 1);
+		 TREE_CODE (olhs) == COMPOUND_EXPR;
+		 olhs = TREE_OPERAND (olhs, 1))
+	      {
+		*ptr = build2 (COMPOUND_EXPR, TREE_TYPE (cond),
+			       TREE_OPERAND (olhs, 0), *ptr);
+		ptr = &TREE_OPERAND (*ptr, 1);
+	      }
+	  }
 	/* Make sure the code to compute the rhs comes out
 	   before the split.  */
-	if (preeval)
-	  cond = build2 (COMPOUND_EXPR, TREE_TYPE (lhs), preeval, cond);
-	return cond;
+	result = cond;
+	goto ret;
       }
 
     default:
+      lhs = olhs;
       break;
     }
 
@@ -7678,7 +7725,7 @@ cp_build_modify_expr (location_t loc, tr
 	    rhs = convert (lhstype, rhs);
 	  result = build2 (INIT_EXPR, lhstype, lhs, rhs);
 	  TREE_SIDE_EFFECTS (result) = 1;
-	  return result;
+	  goto ret;
 	}
       else if (! MAYBE_CLASS_TYPE_P (lhstype))
 	/* Do the default thing.  */;
@@ -7691,7 +7738,7 @@ cp_build_modify_expr (location_t loc, tr
 	  release_tree_vector (rhs_vec);
 	  if (result == NULL_TREE)
 	    return error_mark_node;
-	  return result;
+	  goto ret;
 	}
     }
   else
@@ -7706,7 +7753,7 @@ cp_build_modify_expr (location_t loc, tr
 	    {
 	      result = objc_maybe_build_modify_expr (lhs, rhs);
 	      if (result)
-		return result;
+		goto ret;
 	    }
 
 	  /* `operator=' is not an inheritable operator.  */
@@ -7720,7 +7767,7 @@ cp_build_modify_expr (location_t loc, tr
 				     complain);
 	      if (result == NULL_TREE)
 		return error_mark_node;
-	      return result;
+	      goto ret;
 	    }
 	  lhstype = olhstype;
 	}
@@ -7765,7 +7812,7 @@ cp_build_modify_expr (location_t loc, tr
 	    {
 	      result = objc_maybe_build_modify_expr (lhs, newrhs);
 	      if (result)
-		return result;
+		goto ret;
 	    }
 	}
       gcc_assert (TREE_CODE (lhstype) != REFERENCE_TYPE);
@@ -7861,9 +7908,10 @@ cp_build_modify_expr (location_t loc, tr
 
       from_array = TREE_CODE (TREE_TYPE (newrhs)) == ARRAY_TYPE
 		   ? 1 + (modifycode != INIT_EXPR): 0;
-      return build_vec_init (lhs, NULL_TREE, newrhs,
-			     /*explicit_value_init_p=*/false,
-			     from_array, complain);
+      result = build_vec_init (lhs, NULL_TREE, newrhs,
+			       /*explicit_value_init_p=*/false,
+			       from_array, complain);
+      goto ret;
     }
 
   if (modifycode == INIT_EXPR)
@@ -7902,7 +7950,7 @@ cp_build_modify_expr (location_t loc, tr
       result = objc_generate_write_barrier (lhs, modifycode, newrhs);
 
       if (result)
-	return result;
+	goto ret;
     }
 
   result = build2 (modifycode == NOP_EXPR ? MODIFY_EXPR : INIT_EXPR,
@@ -7912,6 +7960,9 @@ cp_build_modify_expr (location_t loc, tr
   if (!plain_assign)
     TREE_NO_WARNING (result) = 1;
 
+ ret:
+  if (preeval)
+    result = build2 (COMPOUND_EXPR, TREE_TYPE (result), preeval, result);
   return result;
 }
 
--- gcc/testsuite/g++.dg/cpp1z/eval-order4.C.jj	2017-02-13 18:11:00.744134797 +0100
+++ gcc/testsuite/g++.dg/cpp1z/eval-order4.C	2017-02-13 18:11:00.744134797 +0100
@@ -0,0 +1,80 @@
+// PR c++/79232
+// { dg-do run }
+// { dg-options "-fstrong-eval-order" }
+
+int last = 0;
+
+int
+foo (int i)
+{
+  if (i != last + 1)
+    __builtin_abort ();
+  last = i;
+  return i;
+}
+
+char a, b;
+int c;
+
+char &
+bar (int i, int j)
+{
+  foo (i);
+  return j ? a : b;
+}
+
+int
+main ()
+{
+  (foo (2) ? bar (3, 0) : bar (3, 1)) = foo (1);
+  if (last != 3)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3) ? bar (4, 0) : bar (4, 1)) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3) ? bar (4, 0) : bar (4, 1))) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3), foo (4) ? bar (5, 0) : bar (5, 1)) = foo (1);
+  if (last != 5)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3), (foo (4) ? bar (5, 0) : bar (5, 1)))) = foo (1);
+  if (last != 5)
+    __builtin_abort ();
+  last = 0;
+  --c = foo (1);
+  if (c != 1)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), --c) = foo (1);
+  if (last != 2 || c != 1)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3), --c) = foo (1);
+  if (last != 3 || c != 1)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3), --c)) = foo (1);
+  if (last != 3 || c != 1)
+    __builtin_abort ();
+  last = 0;
+  bar (2, 0) = foo (1);
+  if (last != 2)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), bar (3, 0)) = foo (1);
+  if (last != 3)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), foo (3), bar (4, 0)) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+  last = 0;
+  (foo (2), (foo (3), bar (4, 0))) = foo (1);
+  if (last != 4)
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/other/pr79232.C.jj	2017-02-13 18:11:00.745134784 +0100
+++ gcc/testsuite/g++.dg/other/pr79232.C	2017-02-13 18:11:00.745134784 +0100
@@ -0,0 +1,12 @@
+// PR c++/79232
+// { dg-do compile }
+
+extern char a[];
+int b;
+char c, e;
+
+void
+foo (long d)
+{
+  (0, b ? &c : a)[d] = e;
+}


	Jakub

Re: Patch ping^2

[Jakub Jelinek]

On Mon, Feb 13, 2017 at 11:41:48AM -0500, Nathan Sidwell wrote:
> On 02/13/2017 10:46 AM, Jakub Jelinek wrote:
> > Hi!
> > 
> > I'd like to ping a couple of patches:
> > 
> > - C++ P1 PR79232 - ICEs and wrong-code with COMPOUND_EXPR on lhs of assignment
> >   http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02341.html
> 
> What puzzles me about (and may be an existing orthogonal issue), is the
> checking for TYPE(rhs) == VOID.  In the current code it's only explicitly
> checked in the lhs == COND_EXPR case, which is strange.  Why isn't it a
> general constraint?

I'll double check; copied this from the COND_EXPR case which had the same.
I believe the reason is that this is (or ought to be) checked later on,
but stabilitize_expr wouldn't work well if it is called with a void
expression.

	Jakub

Re: Patch ping^2

[Nathan Sidwell]

On 02/13/2017 10:46 AM, Jakub Jelinek wrote:
> Hi!
>
> I'd like to ping a couple of patches:
>
> - C++ P1 PR79232 - ICEs and wrong-code with COMPOUND_EXPR on lhs of assignment
>   http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02341.html

What puzzles me about (and may be an existing orthogonal issue), is the 
checking for TYPE(rhs) == VOID.  In the current code it's only 
explicitly checked in the lhs == COND_EXPR case, which is strange.  Why 
isn't it a general constraint?

nathan

-- 
Nathan Sidwell

Patch ping^2

[Jakub Jelinek]

Hi!

I'd like to ping a couple of patches:

- C++ P1 PR79232 - ICEs and wrong-code with COMPOUND_EXPR on lhs of assignment
  http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02341.html

- C++ P1 PR79288 - wrong default TLS model for __thread static data members
  http://gcc.gnu.org/ml/gcc-patches/2017-01/msg02349.html

- small simplification for gimple-ssa-sprintf.c
  http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00331.html

- noipa attribute addition
  http://gcc.gnu.org/ml/gcc-patches/2016-12/msg01501.html

	Jakub

Patch ping^2

[Jakub Jelinek]

http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01100.html                                                                                            
  - C++ -Wsizeof-pointer-memaccess support (C is already in)                                                                                       

	Jakub

Re: Patch ping #2

[Richard Henderson]

On 05/31/2011 02:19 AM, Jakub Jelinek wrote:
> Hi!
> 
> - http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01182.html                                                                                          
>   various debug info improvements (typed DWARF stack etc.)                                                                                         

Ok.  Although it might be good to break up mem_loc_descriptor.
You've got some rather big implementations of operations now.

> - http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01246.html                                                                                          
>   optimize away unneeded DW_OP_GNU_convert ops with typed DWARF stack                                                                              

Ok.

> - http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01669.html                                                                                          
>   PR48688 optimization, I know Richard asked for trying it during                                                                                  
>   combine, but that attempt failed due to opposite optimization                                                                                    

Ok.


r~

Patch ping #2

[Jakub Jelinek]

Hi!

- http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01182.html                                                                                          
  various debug info improvements (typed DWARF stack etc.)                                                                                         
                                                                                                                                                   
- http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01246.html                                                                                          
  optimize away unneeded DW_OP_GNU_convert ops with typed DWARF stack                                                                              
                                                                                                                                                   
- http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01669.html                                                                                          
  PR48688 optimization, I know Richard asked for trying it during                                                                                  
  combine, but that attempt failed due to opposite optimization                                                                                    

	Jakub

Re: Patch ping #2

[Richard Guenther]

On Wed, May 25, 2011 at 12:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yes, I mean when -fstack-usage is enabled.  Thus, make
>> -fstack-usage -Wframe-larger-than behave the same as
>> -fstack-usage -Wstack-usage.  Or do you say that -Wstack-usage
>> doesn't require -fstack-usage?
>
> Yes, -Wstack-usage doesn't require -fstack-usage.
>
>> If it doesn't then I hope -Wstack-usage does not have effects on code
>> generation?
>
> Neither -fstack-usage nor -Wstack-usage has any effect on code generation.
> The former generates a .su file and the latter issues a warning.
>
>> And if not then why can't -Wframe-larger-than just be more precise on some
>> targets?
>
> -Wframe-larger-than is documented to work on any targets and to be imprecise.
> So we would need to list the targets for which it is precise (or the targets
> for which it isn't precise) and maintain it.  By contrast, if -Wstack-usage
> returns something, then the answer is always precise.  Moreover I think that
> the common name is a big advantage.

Thanks for explaining.  The patch is ok.

Thanks,
Richard.

> --
> Eric Botcazou
>

Re: Patch ping #2

[Eric Botcazou]

> Yes, I mean when -fstack-usage is enabled.  Thus, make
> -fstack-usage -Wframe-larger-than behave the same as
> -fstack-usage -Wstack-usage.  Or do you say that -Wstack-usage
> doesn't require -fstack-usage?

Yes, -Wstack-usage doesn't require -fstack-usage.

> If it doesn't then I hope -Wstack-usage does not have effects on code
> generation?

Neither -fstack-usage nor -Wstack-usage has any effect on code generation.
The former generates a .su file and the latter issues a warning.

> And if not then why can't -Wframe-larger-than just be more precise on some
> targets?   

-Wframe-larger-than is documented to work on any targets and to be imprecise.
So we would need to list the targets for which it is precise (or the targets 
for which it isn't precise) and maintain it.  By contrast, if -Wstack-usage 
returns something, then the answer is always precise.  Moreover I think that 
the common name is a big advantage.

-- 
Eric Botcazou

Re: Patch ping #2

[Richard Guenther]

On Mon, May 23, 2011 at 12:15 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> > Fix annoying gcov filename handling (gcov.c, 2 lines):
>> >  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01380.html
>>
>> Ok.
>
> Thanks!
>
>> > Introduce -Wstack-usage:
>> >  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01992.html
>>
>> I wonder if we can't simply use -Wframe-larger-than but with
>> the more precise information when -fstack-usage is present.
>
> Do you mean when -fstack-usage is enabled?  This will generate the .su file as
> a by-product.  The advantage of -Wstack-usage is that it has the same name as
> its sibling flag so you can pass -fstack-usage -Wstack-usage to the compiler.

Yes, I mean when -fstack-usage is enabled.  Thus, make
-fstack-usage -Wframe-larger-than behave the same as
-fstack-usage -Wstack-usage.  Or do you say that -Wstack-usage
doesn't require -fstack-usage?  If it doesn't then I hope -Wstack-usage
does not have effects on code generation?  And if not then why
can't -Wframe-larger-than just be more precise on some targets?

Richard.

> --
> Eric Botcazou
>

Re: Patch ping #2

[Eric Botcazou]

> > Fix annoying gcov filename handling (gcov.c, 2 lines):
> >  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01380.html
>
> Ok.

Thanks!

> > Introduce -Wstack-usage:
> >  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01992.html
>
> I wonder if we can't simply use -Wframe-larger-than but with
> the more precise information when -fstack-usage is present.

Do you mean when -fstack-usage is enabled?  This will generate the .su file as 
a by-product.  The advantage of -Wstack-usage is that it has the same name as 
its sibling flag so you can pass -fstack-usage -Wstack-usage to the compiler.

-- 
Eric Botcazou

Re: Patch ping #2

[Richard Guenther]

On Mon, May 23, 2011 at 9:59 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Not as many as Jakub, but still. :-)
>
>
> Fix annoying gcov filename handling (gcov.c, 2 lines):
>  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01380.html

Ok.

> Introduce -Wstack-usage:
>  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01992.html

I wonder if we can't simply use -Wframe-larger-than but with
the more precise information when -fstack-usage is present.

I'll leave dwarf2out.c bits to others.

Thanks,
Richard.

Patch ping #2

[Eric Botcazou]

Not as many as Jakub, but still. :-)


Fix annoying gcov filename handling (gcov.c, 2 lines):
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01380.html

Introduce -Wstack-usage:
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01992.html

Extend TYPE_DECL_IS_STUB trick (dwarf2out.c, 1 line):
  http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00683.html

Emit DW_AT_artificial for types:
  http://gcc.gnu.org/ml/gcc-patches/2011-04/msg00700.html

Fix PR lto/48492 (bis) (dwarf2out.c, 1 line):
  http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01461.html

Thanks in advance.


-- 
Eric Botcazou

patch ping^2

[Jerry Quinn]

http://gcc.gnu.org/ml/gcc-patches/2010-02/msg00222.html

Patch ping^2

[Jakub Jelinek]

On Tue, Feb 14, 2006 at 12:19:41PM -0500, Jakub Jelinek wrote:
> --with{,out}-long-double-128 configure option:
> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg00664.html
> 
> (together with the already approved remaining parts of:
> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg00274.html
> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg00271.html
> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg00263.html).
> 
> Ok for trunk?

I'm attaching the full patch that includes also the approved
parts.
Ok?

2006-02-20  Jakub Jelinek  <jakub@redhat.com>

	* configure.ac: Add --with{,out}-long-double-128 configure option.
	(TARGET_DEFAULT_LONG_DOUBLE_128): New test.
	* configure: Rebuilt.
	* config.in: Rebuilt.
	* doc/install.texi (Options specification): Document
	--with-long-double-128.

	* config/rs6000/linux.h [TARGET_DEFAULT_LONG_DOUBLE_128]
	(RS6000_DEFAULT_LONG_DOUBLE_SIZE): Define to 128.
	* config/rs6000/linux64.h [TARGET_DEFAULT_LONG_DOUBLE_128]
	(RS6000_DEFAULT_LONG_DOUBLE_SIZE): Define to 128.

2006-02-20  Aldy Hernandez  <aldyh@redhat.com>

	* config/s390/s390.c (override_options): Handle
	TARGET_DEFAULT_LONG_DOUBLE_128.

	* config/alpha/alpha.c (override_options): Handle
	TARGET_DEFAULT_LONG_DOUBLE_128.

	* config/sparc/sparc.c (sparc_override_options): Handle
	TARGET_DEFAULT_LONG_DOUBLE_128.

--- gcc/doc/install.texi.jj	2006-02-20 20:01:07.000000000 +0100
+++ gcc/doc/install.texi	2006-02-20 20:21:09.000000000 +0100
@@ -1251,6 +1251,14 @@ extension.  This is enabled by default o
 systems.  Other systems may also support it, but require the user to
 specifically enable it.
 
+@item --with-long-double-128
+Specify if @code{long double} type should be 128-bit by default on selected
+GNU/Linux architectures.  If using @code{--without-long-double-128},
+@code{long double} will be by default 64-bit, the same as @code{double} type.
+When neither of these configure options are used, the default will be
+128-bit @code{long double} when built against GNU C Library 2.4 and later,
+64-bit @code{long double} otherwise.
+
 @end table
 
 @subheading Cross-Compiler-Specific Options
--- gcc/config/alpha/alpha.c.jj	2006-02-13 22:21:21.000000000 +0100
+++ gcc/config/alpha/alpha.c	2006-02-20 20:38:27.000000000 +0100
@@ -516,6 +516,11 @@ override_options (void)
       REAL_MODE_FORMAT (DFmode) = &vax_g_format;
       REAL_MODE_FORMAT (TFmode) = NULL;
     }
+
+#ifdef TARGET_DEFAULT_LONG_DOUBLE_128
+  if (!(target_flags_explicit & MASK_LONG_DOUBLE_128))
+    target_flags |= MASK_LONG_DOUBLE_128;
+#endif
 }
 \f
 /* Returns 1 if VALUE is a mask that contains full bytes of zero or ones.  */
--- gcc/config/s390/s390.c.jj	2006-02-20 20:01:07.000000000 +0100
+++ gcc/config/s390/s390.c	2006-02-20 20:38:27.000000000 +0100
@@ -1415,6 +1415,11 @@ override_options (void)
     }
   else if (s390_stack_guard)
     error ("-mstack-guard implies use of -mstack-size"); 
+
+#ifdef TARGET_DEFAULT_LONG_DOUBLE_128
+  if (!(target_flags_explicit & MASK_LONG_DOUBLE_128))
+    target_flags |= MASK_LONG_DOUBLE_128;
+#endif
 }
 
 /* Map for smallest class containing reg regno.  */
--- gcc/config/sparc/sparc.c.jj	2006-02-13 22:21:22.000000000 +0100
+++ gcc/config/sparc/sparc.c	2006-02-20 20:38:27.000000000 +0100
@@ -791,6 +791,11 @@ sparc_override_options (void)
       sparc_costs = &ultrasparc3_costs;
       break;
     };
+
+#ifdef TARGET_DEFAULT_LONG_DOUBLE_128
+  if (!(target_flags_explicit & MASK_LONG_DOUBLE_128))
+    target_flags |= MASK_LONG_DOUBLE_128;
+#endif
 }
 \f
 #ifdef SUBTARGET_ATTRIBUTE_TABLE
--- gcc/config/rs6000/linux.h.jj	2006-02-18 19:55:52.000000000 +0100
+++ gcc/config/rs6000/linux.h	2006-02-20 20:38:27.000000000 +0100
@@ -120,3 +120,8 @@
 #endif
 
 #define POWERPC_LINUX
+
+/* ppc linux has 128-bit long double support in glibc 2.4 and later.  */
+#ifdef TARGET_DEFAULT_LONG_DOUBLE_128
+#define RS6000_DEFAULT_LONG_DOUBLE_SIZE 128
+#endif
--- gcc/config/rs6000/linux64.h.jj	2006-02-18 19:55:52.000000000 +0100
+++ gcc/config/rs6000/linux64.h	2006-02-20 20:38:27.000000000 +0100
@@ -584,3 +584,8 @@ while (0)
 #endif
 
 #define POWERPC_LINUX
+
+/* ppc{32,64} linux has 128-bit long double support in glibc 2.4 and later.  */
+#ifdef TARGET_DEFAULT_LONG_DOUBLE_128
+#define RS6000_DEFAULT_LONG_DOUBLE_SIZE 128
+#endif
--- gcc/configure.ac.jj	2006-02-20 20:01:08.000000000 +0100
+++ gcc/configure.ac	2006-02-20 20:21:09.000000000 +0100
@@ -3176,6 +3176,39 @@ if test x$gcc_cv_libc_provides_ssp = xye
 	    [Define if your target C library provides stack protector support])
 fi
 
+# Check if TFmode long double should be used by default or not.
+# Some glibc targets used DFmode long double, but with glibc 2.4
+# and later they can use TFmode.
+case "$target" in
+  powerpc*-*-*gnu* | \
+  sparc*-*-linux* | \
+  s390*-*-linux* | \
+  alpha*-*-linux*)
+    AC_ARG_WITH(long-double-128,
+[  --with-long-double-128  Use 128-bit long double by default.],
+      gcc_cv_target_ldbl128="$with_long_double_128",
+      [gcc_cv_target_ldbl128=no
+      if test x$host != x$target || test "x$TARGET_SYSTEM_ROOT" != x; then
+	if test "x$with_sysroot" = x; then
+	  glibc_header_dir="${exec_prefix}/${target_noncanonical}/sys-include"
+	elif test "x$with_sysroot" = xyes; then
+	  glibc_header_dir="${exec_prefix}/${target_noncanonical}/sys-root/usr/include"
+	else
+	  glibc_header_dir="${with_sysroot}/usr/include"
+	fi
+      else
+	glibc_header_dir=/usr/include
+      fi
+      grep '^[ 	]*#[ 	]*define[ 	][ 	]*__LONG_DOUBLE_MATH_OPTIONAL' \
+        $glibc_header_dir/bits/wordsize.h > /dev/null 2>&1 \
+      && gcc_cv_target_ldbl128=yes])
+    ;;
+esac
+if test x$gcc_cv_target_ldbl128 = xyes; then
+  AC_DEFINE(TARGET_DEFAULT_LONG_DOUBLE_128, 1,
+	    [Define if TFmode long double should be the default])
+fi
+
 # Find out what GC implementation we want, or may, use.
 AC_ARG_WITH(gc,
 [  --with-gc={page,zone}   choose the garbage collection mechanism to use
--- gcc/configure.jj	2006-02-20 20:01:08.000000000 +0100
+++ gcc/configure	2006-02-20 20:21:27.000000000 +0100
@@ -932,6 +932,7 @@ Optional Packages:
   --with-libiconv-prefix[=DIR]  search for libiconv in DIR/include and DIR/lib
   --without-libiconv-prefix     don't search for libiconv in includedir and libdir
   --with-system-libunwind use installed libunwind
+  --with-long-double-128  Use 128-bit long double by default.
   --with-gc={page,zone}   choose the garbage collection mechanism to use
                           with the compiler
   --with-system-zlib      use installed libz
@@ -7612,7 +7613,7 @@ if test "${gcc_cv_prog_makeinfo_modern+s
 else
     ac_prog_version=`$MAKEINFO --version 2>&1 |
                    sed -n 's/^.*GNU texinfo.* \([0-9][0-9.]*\).*$/\1/p'`
-  echo "configure:7615: version of makeinfo is $ac_prog_version" >&5
+  echo "configure:7616: version of makeinfo is $ac_prog_version" >&5
   case $ac_prog_version in
     '')     gcc_cv_prog_makeinfo_modern=no;;
     4.[4-9]*)
@@ -15675,6 +15676,46 @@ _ACEOF
 
 fi
 
+# Check if TFmode long double should be used by default or not.
+# Some glibc targets used DFmode long double, but with glibc 2.4
+# and later they can use TFmode.
+case "$target" in
+  powerpc*-*-*gnu* | \
+  sparc*-*-linux* | \
+  s390*-*-linux* | \
+  alpha*-*-linux*)
+
+# Check whether --with-long-double-128 or --without-long-double-128 was given.
+if test "${with_long_double_128+set}" = set; then
+  withval="$with_long_double_128"
+  gcc_cv_target_ldbl128="$with_long_double_128"
+else
+  gcc_cv_target_ldbl128=no
+      if test x$host != x$target || test "x$TARGET_SYSTEM_ROOT" != x; then
+	if test "x$with_sysroot" = x; then
+	  glibc_header_dir="${exec_prefix}/${target_noncanonical}/sys-include"
+	elif test "x$with_sysroot" = xyes; then
+	  glibc_header_dir="${exec_prefix}/${target_noncanonical}/sys-root/usr/include"
+	else
+	  glibc_header_dir="${with_sysroot}/usr/include"
+	fi
+      else
+	glibc_header_dir=/usr/include
+      fi
+      grep '^ 	*#[ 	]*define[ 	][ 	]*__LONG_DOUBLE_MATH_OPTIONAL' \
+        $glibc_header_dir/bits/wordsize.h > /dev/null 2>&1 \
+      && gcc_cv_target_ldbl128=yes
+fi;
+    ;;
+esac
+if test x$gcc_cv_target_ldbl128 = xyes; then
+
+cat >>confdefs.h <<\_ACEOF
+#define TARGET_DEFAULT_LONG_DOUBLE_128 1
+_ACEOF
+
+fi
+
 # Find out what GC implementation we want, or may, use.
 
 # Check whether --with-gc or --without-gc was given.
--- gcc/config.in.jj	2005-12-27 17:31:45.000000000 +0100
+++ gcc/config.in	2006-02-20 20:21:09.000000000 +0100
@@ -1283,6 +1283,12 @@
 #endif
 
 
+/* Define if TFmode long double should be the default */
+#ifndef USED_FOR_TARGET
+#undef TARGET_DEFAULT_LONG_DOUBLE_128
+#endif
+
+
 /* Define if your target C library provides stack protector support */
 #ifndef USED_FOR_TARGET
 #undef TARGET_LIBC_PROVIDES_SSP


	Jakub

Patch ping**2

[Ben Elliston]

Ping:
  http://gcc.gnu.org/ml/gcc-patches/2005-12/msg00472.html

If this patch is too hard to review in its present form, is there
further partitioning that would make it easier for reviewers?

Thanks,
Ben

Patch ping^2

[Uros Bizjak]

Hello!

  I would relly appreciate a revew of a simple one-liner patch that 
would fix critical regression for i386 in 4.0.x and 4.1 branches.  The 
patch itself can be found at 
http://gcc.gnu.org/ml/gcc-patches/2005-07/msg01490.html. There are also 
some (unreviewed) testcases for recent PR fixes at 
http://gcc.gnu.org/ml/gcc-patches/2005-08/msg00966.html.

  BTW: I may be mistaken, but I was under impression that regressions 
(found in real code, not some artifical testcases) that are marked 
critical for some reason, have some kind of priority. Otherwise,  I see 
no point to fix them in as-soon-as-possible way, if the patch is then 
left unreviewed for a month.

Thanks,
Uros.

Re: Patch ping**2

[Steve Kargl]

On Sun, Mar 27, 2005 at 08:17:02PM +0200, Tobias Schl?ter wrote:
> 
> 2005-03-08 [gfortran] Fix PR 20178: Implement g77 / f2c calling conventions
> mail here: http://gcc.gnu.org/ml/fortran/2005-03/msg00126.html
> patch here: http://gcc.gnu.org/ml/fortran/2005-03/msg00129.html
> one-line fix here: http://gcc.gnu.org/ml/fortran/2005-03/msg00163.html

Has Steven Johnson tested this patch?  He was the most out 
spoken proponent of this change.  I know you've bootstrapped
and regression tested the patch, but has anyone actually
used gfortran with a g77 compiled library?

> 2005-03-05 [gfortran] More type-safety issues
> http://gcc.gnu.org/ml/fortran/2005-03/msg00071.html
> (the part which removes the fold_convert doesn't work with the g77 calling
> conventions, but I think the should remove support for the NULL rhs anyway,
> and that part is independent)

The independent portion is okay.  I still need to investigate
the g77 calling convention issue.

-- 
Steve

Re: Patch ping**2

[Feng Wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb2312, Size: 761 bytes --]

> 2005-03-05 [gfortran] More type-safety issues
> http://gcc.gnu.org/ml/fortran/2005-03/msg00071.html
> (the part which removes the fold_convert doesn't work with the g77 calling
> conventions, but I think the should remove support for the NULL rhs anyway,
> and that part is independent)
> 
I agree with this one. It's more reasonable.

Best Regards,
Feng Wang

_________________________________________________________
Do You Yahoo!?
150ÍòÇúMP3·è¿ñËÑ£¬´øÄú´³ÈëÒôÀÖµîÌÃ
http://music.yisou.com/
ÃÀÅ®Ã÷ÐÇÓ¦Óо¡ÓУ¬ËѱéÃÀͼ¡¢ÑÞͼºÍ¿áͼ
http://image.yisou.com
1G¾ÍÊÇ1000Õ×£¬ÑÅ»¢µçÓÊ×ÔÖúÀ©ÈÝ£¡
http://cn.rd.yahoo.com/mail_cn/tag/1g/*http://cn.mail.yahoo.com/event/mail_1g/

Patch ping**2

[Tobias Schlüter]

2005-03-08 [gfortran] Fix PR 20178: Implement g77 / f2c calling conventions
mail here: http://gcc.gnu.org/ml/fortran/2005-03/msg00126.html
patch here: http://gcc.gnu.org/ml/fortran/2005-03/msg00129.html
one-line fix here: http://gcc.gnu.org/ml/fortran/2005-03/msg00163.html

2005-03-05 [gfortran] More type-safety issues
http://gcc.gnu.org/ml/fortran/2005-03/msg00071.html
(the part which removes the fold_convert doesn't work with the g77 calling
conventions, but I think the should remove support for the NULL rhs anyway,
and that part is independent)

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

> >And why do you assume that the direction in that the patch takes it is
> >wrong?  It is the way at least one other compiler (LLVM) uses.
> 
> Not quite. At least, I don't believe it's quite the same.
> LLVM will hoist loads/sink stores if the addressing arithmetic for a 
> given load/store doesn't change.
> It doesn't look like you do that anymore, but i can't quite tell.
> IE they use the equivalent of single_reachable_address, called 
> PointerInvalidatedByLoop for loads, and an is_loop_invariant check for 
> stores, both of which it looks like you've removed (by removing 
> single_reachable_address).

PointerInvalidatedByLoop is equivalent to what the rewritten lim does using
count_tag_references and the check at the start of determine_lsm_ref,
given the limited amount of aliasing information we have now.

Moving of loads for that there are no aliasing stores is not part of
store motion and it is performed in
determine_invariantness/move_computations (using the virtual ssa).

> > It is
> >clean and very easy to understand and extend.  Also once the
> >msg01437.html is accepted (which it should, at least for 4.1, although 
> >I
> >think it is a bit too intrusive for 4.0), it is trivial to make it take
> >advantage of SSA form for virtual operands.
> >
> 
> I don't see how it would look much different from what we have now if 
> you did.
> I'd appreciate it if you could show me what you have in mind.

basically replacing the count_tag_references/determine_lsm_ref cycle by
traversing the UD chains in virtual operands from loads and stores in
the loop body.

Indeed it is fairly similar to what we do now, but it would not need to
scan also through DU chains as we do now (which seems to be one of the
reasons for compile time problems, since many of the uses we look at are
outside of the loop and therefore they are not interesting). It also would
not mix the steps of finding the memory references and determining their aliasing
relationships in a single relatively hard to understand function
(single_reachable_address).

Zdenek

Re: Patch ping ^ 2

[Daniel Berlin]

On Oct 18, 2004, at 10:25 AM, Zdenek Dvorak wrote:

> Hello,
>
>>> 1) Unless your patch to make voperands represent output dependences 
>>> again
>>>    (http://gcc.gnu.org/ml/gcc-patches/2004-10/msg01437.html) gets 
>>> into
>>>    mainline, the rewrite of store motion
>>>    (http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html) is the 
>>> best
>>>    choice.
>>
>> 	Regardless whether Dan's patch gets approved for mainline, the
>> original, unmodified patch is taking GCC in the wrong direction and 
>> should
>> not be accepted.
>
> you cannot be serious.  Unless the msg01437.html patch is accepted, the
> rewrite is the only sane possibility how to solve PR 17133 (serious
> misscompilations...)
This is almost true.
The other option is to disable LIM for 4.0 until it's fixed, if the 
must-def patch doesn't go in.
However, you are correct that without that killing information, there 
would be no good way to fix 17133 in 4.0 besides disabling or your 
patch.


>
> And why do you assume that the direction in that the patch takes it is
> wrong?  It is the way at least one other compiler (LLVM) uses.

Not quite. At least, I don't believe it's quite the same.
LLVM will hoist loads/sink stores if the addressing arithmetic for a 
given load/store doesn't change.
It doesn't look like you do that anymore, but i can't quite tell.
IE they use the equivalent of single_reachable_address, called 
PointerInvalidatedByLoop for loads, and an is_loop_invariant check for 
stores, both of which it looks like you've removed (by removing 
single_reachable_address).

Also, LLVM does it this way because they *don't* have a virtual form 
for memory operands.

Open64 basically does what we were doing, except even more complex 
because it's part of SSAPRE on the reverse flowgraph (they nicknamed it 
SSUPRE, for single static use. They build an entirely new ssa form for 
stores), and SSAPRE is insanely complex.

IBM's compiler does it more or less like we do now (since they have 
virtual ssa form), only in ~500 lines of code. :)


>  It is
> clean and very easy to understand and extend.  Also once the
> msg01437.html is accepted (which it should, at least for 4.1, although 
> I
> think it is a bit too intrusive for 4.0), it is trivial to make it take
> advantage of SSA form for virtual operands.
>

I don't see how it would look much different from what we have now if 
you did.
I'd appreciate it if you could show me what you have in mind.


> Zdenek

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

> > 1) Unless your patch to make voperands represent output dependences again
> >    (http://gcc.gnu.org/ml/gcc-patches/2004-10/msg01437.html) gets into
> >    mainline, the rewrite of store motion
> >    (http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html) is the best
> >    choice.
> 
> 	Regardless whether Dan's patch gets approved for mainline, the
> original, unmodified patch is taking GCC in the wrong direction and should
> not be accepted.

you cannot be serious.  Unless the msg01437.html patch is accepted, the
rewrite is the only sane possibility how to solve PR 17133 (serious
misscompilations...)

And why do you assume that the direction in that the patch takes it is
wrong?  It is the way at least one other compiler (LLVM) uses.  It is
clean and very easy to understand and extend.  Also once the
msg01437.html is accepted (which it should, at least for 4.1, although I
think it is a bit too intrusive for 4.0), it is trivial to make it take
advantage of SSA form for virtual operands.

Zdenek

Re: Patch ping ^ 2

[David Edelsohn]

> 1) Unless your patch to make voperands represent output dependences again
>    (http://gcc.gnu.org/ml/gcc-patches/2004-10/msg01437.html) gets into
>    mainline, the rewrite of store motion
>    (http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html) is the best
>    choice.

	Regardless whether Dan's patch gets approved for mainline, the
original, unmodified patch is taking GCC in the wrong direction and should
not be accepted.

	Developing loop optimizations for the GCC SSA infrastructure in
isolation without utilizing the assistance of other developer is much less
effective and much less productive.

David

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

I have thought about the situation a bit more.  My opinion now is:

1) Unless your patch to make voperands represent output dependences again
   (http://gcc.gnu.org/ml/gcc-patches/2004-10/msg01437.html) gets into
   mainline, the rewrite of store motion
   (http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html) is the best
   choice.

2) If your patch gets to mainline, it will be still necessary to rewrite
   store motion to fix its current compile time problems.  It will
   however be useful to use SSA form for voperands then.  This however
   will be just a fairly small change over
   http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html, since the
   only thing that will need to be changed is the way how the
   information about aliasing is determined.

Any of these options is fine with me, although 2) will be easier to
update when your changes to aliasing info representation are included
(it should just work without any further changes, in fact, but of course
one cannot be completely sure).

Note that there are no real dependences between the two patches, i.e.
they can be reviewed independently.

Zdenek

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

> > > As for your earlier message that:
> > > 
> > > >1) It does not really bring any advantages over conventional
> > > >approaches.
> > > 
> > > This is not true.
> > > For example, you can do a neat form of promotion of entire blocks where
> > > the conditional test is invariant when it is in ssa form that you simply can't do
> > > easily without ssa form.

could you provide an example of what you mean?  I cannot imagine ssa
form for virtual operands would be useful for that.

> > > It would be true to state that your current implementation takes no
> > > special advantage of SSA form, however, it is not true to state that
> > > nobody does store motion better because it's done on SSA form.
> > > 
> > > "2) It is slower than conventional approaches."
> > > Only if you make it so.
> > > Considering the majority of other commercial compilers (IBM, Intel,
> > > Open64) and aggressively optimizing non-commercial compilers (LLVM, for
> > > example) do store motion on SSA, and do so very very quickly, this leads
> > > me to believe that the problem is in your implementation, not with the
> > > algorithm itself.
> > 
> > You are wrong at least about LLVM, for sure.
> 
> Uh, see LICM.cpp.

yes, I did.  It uses *exactly* the same algorithm for store motion as
the rewritten version of tree-ssa-loop-im.c -- i.e. it gathers first
all aliasing information about stores and loads inside loop, then uses
it to decide what memory references can be promoted.  Even the
representation of alias sets used by LICM.cpp is pretty close to the one
used in new tree-ssa-loop-im.c.

> >   LLVM does not represent aliasing info in ssa form.  
> I never said it did.

Well, if you did not, I do not know what you are trying to argue about.
Rewrite of LICM still uses SSA form, only it does not use it to decide
which memory references alias.

Zdenek

> You said store motion was slower on SSA than convential approachs,
> LLVM's is on ssa and it is not slower.

Re: Patch ping ^ 2

[Daniel Berlin]

On Mon, 2004-10-18 at 01:43 +0200, Zdenek Dvorak wrote:
> Hello,

> > Consider:
> > 
> > 
> > a_3 = V_MAY_DEF <a_2, 0, 4>
> > a_4 = V_MAY_DEF <a_2, 4, 8>
> > 
> > If you only look at names, you will move neither store.
> 
> yes, once you start doing this type of changes you do on your branch
> that are completely incompatible with how the system currently works,
> the things will have to be changed.

And making the types of assumptions you are proposing building into LIM
make exactly these types of changes much harder.  Which is why i said if
we go down the route of applying your patch, I would just rewrite LIM.


> > >  I have no idea what you mean by "relies on ... lack of
> > > aliasing".
> > 
> > The whole tag_ref counting thing.
> 
> This just precisely reflects what information on aliasing is currently
> provided.
>   Assuming that your changes will be accepted, this will have
> to be changed (and of course, the ssa form based implementation would
> have to be changed as well)
Not really, the ssa form implementation would just work, unless you've
got some weird stuff hidden in there.
It does just work, actually, AFAIK.

> 
> > As for your earlier message that:
> > 
> > >1) It does not really bring any advantages over conventional
> > >approaches.
> > 
> > This is not true.
> > For example, you can do a neat form of promotion of entire blocks where
> > the conditional edge is when it is in ssa form that you simply can't do
> > easily without ssa form.
> 
> I cannot parse this sentence.
> 

s/conditional edge is/conditional test is invariant/

> > It would be true to state that your current implementation takes no
> > special advantage of SSA form, however, it is not true to state that
> > nobody does store motion better because it's done on SSA form.
> > 
> > "2) It is slower than conventional approaches."
> > Only if you make it so.
> > Considering the majority of other commercial compilers (IBM, Intel,
> > Open64) and aggressively optimizing non-commercial compilers (LLVM, for
> > example) do store motion on SSA, and do so very very quickly, this leads
> > me to believe that the problem is in your implementation, not with the
> > algorithm itself.
> 
> You are wrong at least about LLVM, for sure.

Uh, see LICM.cpp.

>   LLVM does not represent aliasing info in ssa form.  
I never said it did.
You said store motion was slower on SSA than convential approachs,
LLVM's is on ssa and it is not slower.


> I do not know much about other compilers,
> but I suspect gcc is the only one that does attempt to do it.
You'd suspect very wrong.
Open64 has mu and chi nodes to represent aliasing in ssa form, IBM's
compilers have even more types of ssa references for aliasing (they
actually have indirect-ssa uses that also have a pointer to all the ssa
defs that make up the addressing arithmetic for things like
indirect_refs).


> 
> > "3) It is more complicated and harder to understand than conventional
> >    approaches."
> > This is only because you are doing it in a somewhat ad-hoc way.
> > Store motion, is not hard to understand or more complicated in SSA form
> > that it is elsewhere. You have exactly the same legality conditions,
> > it's actually easier to test them.
> 
> I thought that as well, before I tried to implement it.

Did you ever ask for help when you ran into trouble?
I'm sure people would have been more than happy to try to explain how do
to it fast in SSA form.

> 
> Zdenek
-- 

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

> > > > this patch fixes (at least) two several serious problems in lim store
> > > > motion -- misscompilations (PR 17133 and several duplicates) and
> > > > problems with compile time (PR 17790).
> > > > 
> > > > http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html
> > > 
> > > I still have serious problems with this patch.
> > > It simply drops using the SSA form to do store motion, and relies on
> > > variable names and lack of aliasing to decide what to move.  
> > 
> > could you please explain more precisely what you mean by the last two
> > points?  If "relies on variable names" means that it ignores ssa
> > versions for virtual operands, yes, it does, since virtual operands are
> > in FUD form,
> >  so their ssa versions do not carry any interesting
> > information
> >  (if you do not use them to traverse UD chains, which does
> > not make much sense in case you need to scan body of the whole loop
> > anyway). 
> This is only sort of true now (it's only true if you exclude must-defs,
> and assume that our chains don't bypass non-aliasing uses, which they
> admittedly happen to now), and certainly won't be true in the future.
> 
> Consider:
> 
> 
> a_3 = V_MAY_DEF <a_2, 0, 4>
> a_4 = V_MAY_DEF <a_2, 4, 8>
> 
> If you only look at names, you will move neither store.

yes, once you start doing this type of changes you do on your branch
that are completely incompatible with how the system currently works,
the things will have to be changed.

> >  I have no idea what you mean by "relies on ... lack of
> > aliasing".
> 
> The whole tag_ref counting thing.

This just precisely reflects what information on aliasing is currently
provided.  Assuming that your changes will be accepted, this will have
to be changed (and of course, the ssa form based implementation would
have to be changed as well).

> As for your earlier message that:
> 
> >1) It does not really bring any advantages over conventional
> >approaches.
> 
> This is not true.
> For example, you can do a neat form of promotion of entire blocks where
> the conditional edge is when it is in ssa form that you simply can't do
> easily without ssa form.

I cannot parse this sentence.

> It would be true to state that your current implementation takes no
> special advantage of SSA form, however, it is not true to state that
> nobody does store motion better because it's done on SSA form.
> 
> "2) It is slower than conventional approaches."
> Only if you make it so.
> Considering the majority of other commercial compilers (IBM, Intel,
> Open64) and aggressively optimizing non-commercial compilers (LLVM, for
> example) do store motion on SSA, and do so very very quickly, this leads
> me to believe that the problem is in your implementation, not with the
> algorithm itself.

You are wrong at least about LLVM, for sure.  LLVM does not represent
aliasing info in ssa form.  I do not know much about other compilers,
but I suspect gcc is the only one that does attempt to do it.

> "3) It is more complicated and harder to understand than conventional
>    approaches."
> This is only because you are doing it in a somewhat ad-hoc way.
> Store motion, is not hard to understand or more complicated in SSA form
> that it is elsewhere. You have exactly the same legality conditions,
> it's actually easier to test them.

I thought that as well, before I tried to implement it.

Zdenek

Re: Patch ping ^ 2

[Daniel Berlin]

On Sun, 2004-10-17 at 23:43 +0200, Zdenek Dvorak wrote:
> Hello,
> 
> > > this patch fixes (at least) two several serious problems in lim store
> > > motion -- misscompilations (PR 17133 and several duplicates) and
> > > problems with compile time (PR 17790).
> > > 
> > > http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html
> > 
> > I still have serious problems with this patch.
> > It simply drops using the SSA form to do store motion, and relies on
> > variable names and lack of aliasing to decide what to move.  
> 
> could you please explain more precisely what you mean by the last two
> points?  If "relies on variable names" means that it ignores ssa
> versions for virtual operands, yes, it does, since virtual operands are
> in FUD form,
>  so their ssa versions do not carry any interesting
> information
>  (if you do not use them to traverse UD chains, which does
> not make much sense in case you need to scan body of the whole loop
> anyway). 
This is only sort of true now (it's only true if you exclude must-defs,
and assume that our chains don't bypass non-aliasing uses, which they
admittedly happen to now), and certainly won't be true in the future.

Consider:


a_3 = V_MAY_DEF <a_2, 0, 4>
a_4 = V_MAY_DEF <a_2, 4, 8>

If you only look at names, you will move neither store.

>  I have no idea what you mean by "relies on ... lack of
> aliasing".

The whole tag_ref counting thing.

As for your earlier message that:

>1) It does not really bring any advantages over conventional
>approaches.

This is not true.
For example, you can do a neat form of promotion of entire blocks where
the conditional edge is when it is in ssa form that you simply can't do
easily without ssa form.
It would be true to state that your current implementation takes no
special advantage of SSA form, however, it is not true to state that
nobody does store motion better because it's done on SSA form.

"2) It is slower than conventional approaches."
Only if you make it so.
Considering the majority of other commercial compilers (IBM, Intel,
Open64) and aggressively optimizing non-commercial compilers (LLVM, for
example) do store motion on SSA, and do so very very quickly, this leads
me to believe that the problem is in your implementation, not with the
algorithm itself.

"3) It is more complicated and harder to understand than conventional
   approaches."
This is only because you are doing it in a somewhat ad-hoc way.
Store motion, is not hard to understand or more complicated in SSA form
that it is elsewhere. You have exactly the same legality conditions,
it's actually easier to test them.







> 
> Zdenek
-- 

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

> > this patch fixes (at least) two several serious problems in lim store
> > motion -- misscompilations (PR 17133 and several duplicates) and
> > problems with compile time (PR 17790).
> > 
> > http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html
> 
> I still have serious problems with this patch.
> It simply drops using the SSA form to do store motion, and relies on
> variable names and lack of aliasing to decide what to move.  

could you please explain more precisely what you mean by the last two
points?  If "relies on variable names" means that it ignores ssa
versions for virtual operands, yes, it does, since virtual operands are
in FUD form, so their ssa versions do not carry any interesting
information (if you do not use them to traverse UD chains, which does
not make much sense in case you need to scan body of the whole loop
anyway).  I have no idea what you mean by "relies on ... lack of
aliasing".

Zdenek

Re: Patch ping ^ 2

[Zdenek Dvorak]

Hello,

> > this patch fixes (at least) two several serious problems in lim store
> > motion -- misscompilations (PR 17133 and several duplicates) and
> > problems with compile time (PR 17790).
> > 
> > http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html
> 
> I still have serious problems with this patch.
> It simply drops using the SSA form to do store motion, and relies on
> variable names and lack of aliasing to decide what to move.  
> This is a serious pessimization from what we have now, which is only
> broken because you move things past must-defs that kill them, and
> because you don't verify that you aren't doing bad things with global
> variables (you need to do an is_global_var check, and a check like dce
> does for stores to global variables, and then do the additional checks,
> like reverse reachability from the exit node, to make sure you aren't
> making the store invisible to outside users).
> 
> If this patch is approved, I'll likely rewrite store motion as part of
> the sinking work i'm doing so that it works on SSA form again.

I think you are wrong.  The rewrite has exactly the same functionality
as the current code, and should give exactly the same results.

My opinion currently is that using SSA form for virtual operands is not the
best choice for this kind of optimization, the reasons being:

1) It does not really bring any advantages over conventional approaches.
2) It is slower than conventional approaches.
3) It is more complicated and harder to understand than conventional
   approaches.

Zdenek

Re: Patch ping ^ 2

[Daniel Berlin]

On Sun, 2004-10-17 at 22:22 +0200, Zdenek Dvorak wrote:
> Hello,
> 
> this patch fixes (at least) two several serious problems in lim store
> motion -- misscompilations (PR 17133 and several duplicates) and
> problems with compile time (PR 17790).
> 
> http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html

I still have serious problems with this patch.
It simply drops using the SSA form to do store motion, and relies on
variable names and lack of aliasing to decide what to move.  
This is a serious pessimization from what we have now, which is only
broken because you move things past must-defs that kill them, and
because you don't verify that you aren't doing bad things with global
variables (you need to do an is_global_var check, and a check like dce
does for stores to global variables, and then do the additional checks,
like reverse reachability from the exit node, to make sure you aren't
making the store invisible to outside users).

If this patch is approved, I'll likely rewrite store motion as part of
the sinking work i'm doing so that it works on SSA form again.



> 
> Zdenek
-- 

Patch ping ^ 2

[Zdenek Dvorak]

Hello,

this patch fixes (at least) two several serious problems in lim store
motion -- misscompilations (PR 17133 and several duplicates) and
problems with compile time (PR 17790).

http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01120.html

Zdenek

Patch ping ^2

[Pat Haugen]

Ping for crossjump patch.

http://gcc.gnu.org/ml/gcc-patches/2004-07/msg00495.html


-Pat

patch ping^2

[Jerry Quinn]

Still unreviewed:

http://gcc.gnu.org/ml/gcc-patches/2004-06/msg01662.html

Patch ping^2

[Richard Guenther]

Ping!

http://gcc.gnu.org/ml/gcc-patches/2004-05/msg00187.html
[PATCH] Add leafify function attribute

Thanks,
Richard.

Re: Patch ping^2

[Zdenek Dvorak]

Hello,

> > http://gcc.gnu.org/ml/gcc-patches/2004-02/msg01733.html
> >   -- doloop optimization rewrite.
> 
> This is Ok for mainline.

thanks a lot.

> The only reason I didn't approve this patch (with the others) after the
> first ping is because you'd Cc'd "rth at redhat dot com" on the original
> posting and that reminder.  I guessing after two pings, you hadn't
> previously arranged with him to examine this patch?
> 
> My apologies if I've committed a "faux pas" reviewing this patch
> after only two pings, but Cc'ing maintainers creates a etiquette dilema,
> which only slows down the patch review process.  Perhaps using Bcc is
> safer?

oops... sorry, I think it is just a reflex from the times when basically
noone but Richard did approve any patches :-)

Zdenek

Patch ping^2

[Zdenek Dvorak]

Hello,

http://gcc.gnu.org/ml/gcc-patches/2004-02/msg01733.html
  -- doloop optimization rewrite.  I would like to get this in since
     it fixes the problem with interaction of the new loop unroller and
     doloop optimization (possibly not fully optimally, as we discussed
     with Mircea Namolaru, at least in a way that should be simple to
     improve upon).

Zdenek

Re: Patch ping^2

[Roger Sayle]

On Wed, 17 Mar 2004, Zdenek Dvorak wrote:
> http://gcc.gnu.org/ml/gcc-patches/2004-02/msg01733.html
>   -- doloop optimization rewrite.

This is Ok for mainline.


The only reason I didn't approve this patch (with the others) after the
first ping is because you'd Cc'd "rth at redhat dot com" on the original
posting and that reminder.  I guessing after two pings, you hadn't
previously arranged with him to examine this patch?

My apologies if I've committed a "faux pas" reviewing this patch
after only two pings, but Cc'ing maintainers creates a etiquette dilema,
which only slows down the patch review process.  Perhaps using Bcc is
safer?

Roger
--

Re: Patch ping^2

[Zdenek Dvorak]

Hello,

> > http://gcc.gnu.org/ml/gcc-patches/2004-02/msg01733.html
> >   -- doloop optimization rewrite.
> 
> This is Ok for mainline.

thanks a lot.

> The only reason I didn't approve this patch (with the others) after the
> first ping is because you'd Cc'd "rth at redhat dot com" on the original
> posting and that reminder.  I guessing after two pings, you hadn't
> previously arranged with him to examine this patch?
> 
> My apologies if I've committed a "faux pas" reviewing this patch
> after only two pings, but Cc'ing maintainers creates a etiquette dilema,
> which only slows down the patch review process.  Perhaps using Bcc is
> safer?

oops... sorry, I think it is just a reflex from the times when basically
noone but Richard did approve any patches :-)

Zdenek

Re: Patch ping^2

[Roger Sayle]

On Wed, 17 Mar 2004, Zdenek Dvorak wrote:
> http://gcc.gnu.org/ml/gcc-patches/2004-02/msg01733.html
>   -- doloop optimization rewrite.

This is Ok for mainline.


The only reason I didn't approve this patch (with the others) after the
first ping is because you'd Cc'd "rth at redhat dot com" on the original
posting and that reminder.  I guessing after two pings, you hadn't
previously arranged with him to examine this patch?

My apologies if I've committed a "faux pas" reviewing this patch
after only two pings, but Cc'ing maintainers creates a etiquette dilema,
which only slows down the patch review process.  Perhaps using Bcc is
safer?

Roger
--

Patch ping^2

[Zdenek Dvorak]

Hello,

http://gcc.gnu.org/ml/gcc-patches/2004-02/msg01733.html
  -- doloop optimization rewrite.  I would like to get this in since
     it fixes the problem with interaction of the new loop unroller and
     doloop optimization (possibly not fully optimally, as we discussed
     with Mircea Namolaru, at least in a way that should be simple to
     improve upon).

Zdenek