(Partial) Implementation of simplificaiton of CSHIFT
diff mbox

Message ID 20151121200735.GA55464@troutmask.apl.washington.edu
State New
Headers show

Commit Message

Steve Kargl Nov. 21, 2015, 8:07 p.m. UTC
> > >
> > > Perhaps, bootstrap needs to set appropriate warning levels.
> > 
> > https://gcc.gnu.org/ml/gcc-regression/2015-11/msg00648.html
> > 
> 
> See 5 lines up.
> 

Committed.

Comments

Eric Botcazou Nov. 21, 2015, 10:26 p.m. UTC | #1
> +	* simplify.c (gfc_simplify_cshift): Work around bootstrap issues
> +	due to inappropriate warning options.

The warning options are appropriate, any dead code can potentially hide a bug 
and should be flagged; every static analyzer will also do it and we would soon 
have PRs opened with bugzilla if we tolerated it.
Steve Kargl Nov. 21, 2015, 10:39 p.m. UTC | #2
On Sat, Nov 21, 2015 at 11:26:17PM +0100, Eric Botcazou wrote:
> > +	* simplify.c (gfc_simplify_cshift): Work around bootstrap issues
> > +	due to inappropriate warning options.
> 
> The warning options are appropriate, any dead code can potentially hide a bug 
> and should be flagged; every static analyzer will also do it and we would soon 
> have PRs opened with bugzilla if we tolerated it.
> 

I disgree.

If bootstrap is going to enforce -Werror -Wunused-*, then 
this should be the default for any possible invocation of
make.
Gerald Pfeifer Dec. 31, 2015, 1:57 p.m. UTC | #3
On Sat, 21 Nov 2015, Steve Kargl wrote:
>  2015-11-21  Steven G. Kargl  <kargl@gcc.gnu.org>
>  
> 	* simplify.c (gfc_simplify_cshift): Work around bootstrap issues
> 	due to inappropriate warning options. 

> Index: simplify.c
> ===================================================================
> +      /* GCC bootstrap is too stupid to realize that the above code for dm
> +	 is correct.  First, dim can be specified for a rank 1 array.  It is
> +	 not needed in this nor used here.  Second, the code is simply waiting
> +	 for someone to implement rank > 1 simplification.   For now, add a
> +	 pessimization to the code that has a zero valid reason to be here.  */
> +      if (dm > array->rank)
> +	gcc_unreachable ();

I'm not sure this comment is appropriate as is.  At a minimum, it
should list the version of GCC this was introduced for/with.  So,
something like

  "As of GCC 6, when bootstrapping we do not realize that..."

Gerald
Steve Kargl Dec. 31, 2015, 4:13 p.m. UTC | #4
On Thu, Dec 31, 2015 at 09:57:10PM +0800, Gerald Pfeifer wrote:
> On Sat, 21 Nov 2015, Steve Kargl wrote:
> >  2015-11-21  Steven G. Kargl  <kargl@gcc.gnu.org>
> >  
> > 	* simplify.c (gfc_simplify_cshift): Work around bootstrap issues
> > 	due to inappropriate warning options. 
> 
> > Index: simplify.c
> > ===================================================================
> > +      /* GCC bootstrap is too stupid to realize that the above code for dm
> > +	 is correct.  First, dim can be specified for a rank 1 array.  It is
> > +	 not needed in this nor used here.  Second, the code is simply waiting
> > +	 for someone to implement rank > 1 simplification.   For now, add a
> > +	 pessimization to the code that has a zero valid reason to be here.  */
> > +      if (dm > array->rank)
> > +	gcc_unreachable ();
> 
> I'm not sure this comment is appropriate as is.  At a minimum, it
> should list the version of GCC this was introduced for/with.  So,
> something like
> 

I have no intention to change the comment.

Patch
diff mbox

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 230709)
+++ ChangeLog	(working copy)
@@ -1,5 +1,10 @@ 
 2015-11-21  Steven G. Kargl  <kargl@gcc.gnu.org>
 
+	* simplify.c (gfc_simplify_cshift): Work around bootstrap issues
+	due to inappropriate warning options. 
+
+2015-11-21  Steven G. Kargl  <kargl@gcc.gnu.org>
+
 	* simplify.c (gfc_simplify_cshift): Implement simplification of
 	CSHIFT for rank=1 arrays.
 	(gfc_simplify_spread): Remove a FIXME and add error condition.
Index: simplify.c
===================================================================
--- simplify.c	(revision 230709)
+++ simplify.c	(working copy)
@@ -1869,6 +1869,15 @@  gfc_simplify_cshift (gfc_expr *array, gf
   else
     {
       /* FIXME: Deal with rank > 1 arrays.  For now, don't leak memory.  */
+
+      /* GCC bootstrap is too stupid to realize that the above code for dm
+	 is correct.  First, dim can be specified for a rank 1 array.  It is
+	 not needed in this nor used here.  Second, the code is simply waiting
+	 for someone to implement rank > 1 simplification.   For now, add a
+	 pessimization to the code that has a zero valid reason to be here.  */
+      if (dm > array->rank)
+	gcc_unreachable ();
+
       gfc_free_expr (a);
     }