[patch] Fix PHI optimization issue with boolean types

[Eric Botcazou]

Hi,

this is a regression present on the mainline and 6 branch: the compiler now 
generates wrong code for the attached testcase at -O because of an internal 
conflict about boolean types.  The sequence is as follows.  In .mergephi3:

  boolean _22;
  p__enum res;

  :
  if (_22 != 0)
goto ;
  else
goto ;

  :

  :
  # res_17 = PHI <2(8), 0(9), 1(10)>

is turned into:

COND_EXPR in block 9 and PHI in block 11 converted to straightline code.
PHI res_17 changed to factor conversion out from COND_EXPR.
New stmt with CAST that defines res_17.

  boolean _33;

  :
  # _33 = PHI <2(8), _22(9)>
  res_17 = (p__enum) _33;

[...]

  :
  if (res_17 != 0)
goto ;
  else
goto ;

  :
  _12 = res_17 == 2;
  _13 = (integer) _12

in .phiopt1.  So boolean _33 can have value 2.  Later forwprop3 propagates _33
into the uses of res_17:

  :
  if (_33 != 0)
goto ;
  else
goto ;

  :
  _12 = _33 == 2;
  _13 = (integer) _12;

and DOM3 deduces:

  :
  _12 = 0;
  _13 = 0;

because it computes that _33 has value 1 in BB 13 since it's a boolean.

The problem was introduced by the new factor_out_conditional_conversion:

  /* If arg1 is an INTEGER_CST, fold it to new type.  */
  if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
  && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
{
  if (gimple_assign_cast_p (arg0_def_stmt))
new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
  else
return NULL;
}
  else
return NULL

int_fits_type_p is documented as taking only INTEGER_TYPE, but is invoked
on constant 2 and a BOOLEAN_TYPE and returns true.

BOOLEAN_TYPE is special in Ada: it has precision 8 and range [0;255] so the
outcome of int_fits_type_p is not unreasonable.  But this goes against the
various transformations applied to boolean types in the compiler, which all
assume that they can only take values 0 or 1.

Hence the attached fix (which should be a no-op except for Ada), tested on 
x86_64-suse-linux, OK for mainline and 6 branch?


2016-10-18  Eric Botcazou  

* tree.c (int_fits_type_p): Accept only 0 and 1 for boolean types.


2016-10-18  Eric Botcazou  

* gnat.dg/opt59.adb: New test.
* gnat.dg/opt59_pkg.ad[sb]: New helper.

-- 
Eric BotcazouIndex: tree.c
===
--- tree.c	(revision 241294)
+++ tree.c	(working copy)
@@ -9065,8 +9065,8 @@ get_narrower (tree op, int *unsignedp_pt
   return win;
 }
 
-/* Returns true if integer constant C has a value that is permissible
-   for type TYPE (an INTEGER_TYPE).  */
+/* Return true if integer constant C has a value that is permissible
+   for TYPE, an integral type.  */
 
 bool
 int_fits_type_p (const_tree c, const_tree type)
@@ -9075,6 +9075,11 @@ int_fits_type_p (const_tree c, const_tre
   bool ok_for_low_bound, ok_for_high_bound;
   signop sgn_c = TYPE_SIGN (TREE_TYPE (c));
 
+  /* Short-circuit boolean types since various transformations assume that
+ they can only take values 0 and 1.  */
+  if (TREE_CODE (type) == BOOLEAN_TYPE)
+return integer_zerop (c) || integer_onep (c);
+
 retry:
   type_low_bound = TYPE_MIN_VALUE (type);
   type_high_bound = TYPE_MAX_VALUE (type);
-- { dg-do run }
-- { dg-options "-O" }

with Opt59_Pkg; use Opt59_Pkg;

procedure Opt59 is

  type Enum is (Zero, One, Two);

  function Has_True (V : Boolean_Vector) return Boolean is
  begin
 for I in V'Range loop
if V (I) then
   return True;
end if;
 end loop;
 return False;
  end;

  Data1  : constant Boolean_Vector := Get_BV1;
  Data2  : constant Boolean_Vector := Get_BV2;
  Result : Boolean_Vector;

  function F return Enum is
Res  : Enum := Zero;
Set1 : constant Boolean := Has_True (Data1);
Set2 : constant Boolean := Has_True (Data2);
  begin
if Set1 then
  Res := Two;
elsif Set2 then
  Res := One;
end if;
return Res;
  end;

  Val : constant Enum := F;

begin

  for I in Result'Range loop
Result (I) := Data1 (I) or Data2 (I);
  end loop;

  if Val /= Zero then
Test (Val = Two);
  end if;

end;
package body Opt59_Pkg is

  function Get_BV1 return Boolean_Vector is
  begin
return (others => True);
  end;

  function Get_BV2 return Boolean_Vector is
  begin
return (others => False);
  end;

  procedure Test (B : Boolean) is
  begin
if not B then
  raise Program_Error;
end if;
  end;

end Opt59_Pkg;
package Opt59_Pkg is

  type Boolean_Vector is array (1 .. 8) of Boolean;

  function Get_BV1 return Boolean_Vector;

  function Get_BV2 return Boolean_Vector;

  procedure Test (B : Boolean);

end Opt59_Pkg;

Re: [PATCH] Fix cond-expr handling in genmatch

[Richard Biener]

On Mon, 17 Oct 2016, Richard Biener wrote:

> 
> This fixes matching of toplevel (cond (lt @1 @2) ...) as reported by
> Bin to me privately.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

This is what I applied.

Richard.

2016-10-18  Richard Biener  

* genmatch.c (dt_operand::gen_gimple_expr): Use get_name to
get at the operand to look at with TREE_OPERAND for generic
sub-nodes.

Index: gcc/genmatch.c
===
--- gcc/genmatch.c  (revision 241228)
+++ gcc/genmatch.c  (working copy)
@@ -2644,9 +2644,19 @@ dt_operand::gen_gimple_expr (FILE *f, in
  /* ???  If this is a memory operation we can't (and should not)
 match this.  The only sensible operand types are
 SSA names and invariants.  */
- fprintf_indent (f, indent,
- "tree %s = TREE_OPERAND (gimple_assign_rhs1 
(def), %i);\n",
- child_opname, i);
+ if (e->is_generic)
+   {
+ char opname[20];
+ get_name (opname);
+ fprintf_indent (f, indent,
+ "tree %s = TREE_OPERAND (%s, %i);\n",
+ child_opname, opname, i);
+   }
+ else
+   fprintf_indent (f, indent,
+   "tree %s = TREE_OPERAND "
+   "(gimple_assign_rhs1 (def), %i);\n",
+   child_opname, i);
  fprintf_indent (f, indent,
  "if ((TREE_CODE (%s) == SSA_NAME\n",
  child_opname);

Re: [PATCH] (PR 65950) Improve predicate for exit(0);

[Jan Hubicka]

> > Ah, so you have
> >
> > foo () { loop }
> > main()
> > {
> >   if ()
> >{
> >   foo ();
> >   exit (0);
> >}
> > ...
> >   return 0;
> > }
> >
> > and foo is marked cold because its only call is on the path to exit (0)?
> 
> 
> Actually the case I have here is just:
> foo () { loop }
> int main(void)
> {
> .
> foo();
> ...
> exit (0);
> }
> 
> Just an exit at the end of main.
> Basically if we treat exit(0) as a normal function, main becomes hot again.

Yep, it is old noreturn predicate lazynes.  Path is OK

Honza

> 
> Thanks,
> Andrew
> 
> >
> > noreturn prediction is quite aggressive but it works also quite well.  Given
> > you can only detect a very minor fraction of cases (consider exit (0) being
> > in foo itself) I'm not sure that your fix is good progress.  IMHO we might
> > want to switch from 'noreturn' to 'noreturn' + likely error which needs
> > another attribute / auto-discovery and IPA propagation to handle this case
> > better.
> >
> > Anyway, I'll leave the patch to Honza.
> >
> > Richard.
> >
> >> Thanks,
> >> Andrew
> >>
> >>>
> >>> Richard.
> >>>
>  OK?  Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>  Also tested it with SPEC CPU 2006 with no performance regressions.
> 
>  Thanks,
>  Andrew Pinski
> 
>  ChangeLog:
>  * predict.c (is_exit_with_zero_arg): New function.
>  (tree_bb_level_predictions): Don't consider paths leading to exit(0)
>  as nottaken.

Re: PING: new pass to warn on questionable uses of alloca() and VLAs

[Aldy Hernandez]

On 09/13/2016 04:28 PM, Jeff Law wrote:

On 08/19/2016 08:58 AM, Aldy Hernandez wrote:


I'd just drop the /*strict_mode_p*/ comment in both places it appears in
your patch's change to passes.def.  I think we've generally frowned on
those embedded comments, even though some have snuck in.


I've seen a lot of embedded comments throughout GCC, especially in
optional type arguments.  ISTM it makes things clearer for these
parameters.  But hey, I don't care that much.  Fixed.

Yea.  They've crept in.   Long term this kind of stuff should have an
enum or somesuch so that its obvious at both the call site and
implementation site what those arguments to.


Actually when the cast is from a known positive range we don't get a
VR_ANTI_RANGE, we get a proper VR_RANGE.

Ah, in that case there's several of these things that get trivially
cleaned up :-)

Though I bet with some work I might be able to create an ANTI_RANGE that
excludes all negative numbers.   But I don't think it's going to be that
important in practice.  So we just have to make sure we don't
abort/segfault.



I've cleaned this code up a bit and merged some common conditionals.  In
the process, taking a subset of your advice, and cleaning up some things
I've managed to handle 2 cases where I was previously XFAILing.
So...less false positives.  More coverage.  Woo hoo!

Always goodness.






+{
+  gcc_assert (gimple_alloca_call_p (stmt));
+  tree len = gimple_call_arg (stmt, 0);
+  enum alloca_type w = ALLOCA_UNBOUNDED;
+  wide_int min, max;
+
+  gcc_assert (!is_vla || warn_vla_limit > 0);
+  gcc_assert (is_vla || warn_alloca_limit > 0);
+
+  // Adjust warn_alloca_max_size for VLAs, by taking the underlying
+  // type into account.
+  unsigned HOST_WIDE_INT max_size;
+  if (is_vla)
+max_size = (unsigned HOST_WIDE_INT) warn_vla_limit;
+  else
+max_size = (unsigned HOST_WIDE_INT) warn_alloca_limit;
+
+  // Check for the obviously bounded case.
+  if (TREE_CODE (len) == INTEGER_CST)
+{
+  if (tree_to_uhwi (len) > max_size)
+{
+  *assumed_limit = len;
+  return ALLOCA_BOUND_DEFINITELY_LARGE;
+}
+  if (integer_zerop (len))
+return ALLOCA_ARG_IS_ZERO;
+  w = ALLOCA_OK;
+}
+  else if (TREE_CODE (len) != SSA_NAME)
+return ALLOCA_UNBOUNDED;

Hmm, other than INTEGER_CST and SSA_NAME, is there any other nodes we
can get here?   Perhaps we get DECLs and such, particularly when not
optimizing?!?


Nope.  We don't even run without optimization (because we need VRP/range
info).  I added a gcc_unreachable() to make sure and added an
appropriate comment.  It doesn't get triggered on tests or
glibc/binutils builds.

Yea, maybe I was conflating your work at Martin's (the latter of which
can run without optimization).


So this is an interesting tidbit that answers my questions about how we
use alloca_call_type_by_arg.  Essentially this is meant to catch the
merge point for flow control and give a conservative warning.  That's
fine and good.  But ISTM it's really a bit of a hack.  What if we have
something like this:

   X   Y   Z
\  |  /
  \|/
   A
  / \
 /   \
B C
 / \
/   \
   D E


Where the alloca call is in E and the incoming edges to A actually have
useful information about the argument to the alloca call.

ISTM you need to be doing something with the dominator tree here to find
the merge point(s)  where we might know something useful.  And it's this
kind of test that makes me wonder about re-purposing some of the path
analysis code from tree-ssa-uninit.c.  It may be the case that the path
Z->A->C->E is unfeasible, but left in the CFG because duplication to
expose the unfeasible path was unprofitable.  If it turns out that the
only argument that causes problems comes from the edge Z->A, then we
wouldn't want to warn in this case.

  I don't see Andrew's work necessarily being able to solve this
problem.


In my limited testing I've seen that 95% of all cases (I'm pulling this
number out of thin air ;-)) are relatively simple.  Just looking at the
definition of the SSA name in the alloca() call or the immediate
predecessors yields most of the information we need.  I haven't seen
much more complicated things with an actual range.

Understood.  But it's this kind of thing that creates the false
positives that drive people nuts :-)

As I said, I don't necessarily think Andrew's work will resolve this nor
do I think it ought to block this work.  I mostly pointed it out as an
example of the kind of case we're not going to handle well today, but
perhaps could with the tree-ssa-uninit.c framework.





gcc/

* Makefile.in (OBJS): Add gimple-ssa-warn-alloca.o.
* passes.def: Add two instances of pass_walloca.
* tree-pass.h (make_pass_walloca): New.
* gimple-ssa-warn-alloca.c: New file.
* doc/invoke.texi: Document -Walloca, -Walloca-larger-than=, and
-Wvla-larger-than= options.

gcc/c-family/

* c.opt (Walloca): New.
(Walloca-larger-than=): 

Re: [PATCH v2,rs6000] Add built-in function support for Power9 string operations

[David Edelsohn]

This patch broke bootstrap on AIX.

In altivec_init_builtins(), the loop to initialize predicates is
encountering mode1 == SImode.

Thanks, David

[RFC][IPA-VRP] ADDR_EXPR and nonnull

[kugan]

Hi,

While computing jump function value range for pointer, I am wondering if 
we can assume that any tree with ADDR_EXPR will be nonnull.


That is, in cases like:

int arr[10];
foo ([1]);

OR

struct st
{
  int a;
  int b;
};
struct st s2;
foo ();

Attached patch tries to do this. I am not sure if this can be wrong. Any 
thoughts?


Attached patch bootstraps and regression testing didn't introduce any 
new regressions.


Thanks,
Kugan


gcc/ChangeLog:

2016-10-19  Kugan Vivekanandarajah  

* ipa-prop.c (ipa_compute_jump_functions_for_edge): Set
value range to nonull for ADDR_EXPR.

gcc/testsuite/ChangeLog:

2016-10-19  Kugan Vivekanandarajah  

* gcc.dg/ipa/vrp5.c: New test.
* gcc.dg/ipa/vrp6.c: New test.
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 353b638..b795d30 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1670,9 +1670,10 @@ ipa_compute_jump_functions_for_edge (struct 
ipa_func_body_info *fbi,
 
   if (POINTER_TYPE_P (TREE_TYPE (arg)))
{
- if (TREE_CODE (arg) == SSA_NAME
- && param_type
- && get_ptr_nonnull (arg))
+ if ((TREE_CODE (arg) == SSA_NAME
+  && param_type
+  && get_ptr_nonnull (arg))
+ || (TREE_CODE (arg) == ADDR_EXPR))
{
  jfunc->vr_known = true;
  jfunc->m_vr.type = VR_ANTI_RANGE;
diff --git a/gcc/testsuite/gcc.dg/ipa/vrp5.c b/gcc/testsuite/gcc.dg/ipa/vrp5.c
index e69de29..8e43a65 100644
--- a/gcc/testsuite/gcc.dg/ipa/vrp5.c
+++ b/gcc/testsuite/gcc.dg/ipa/vrp5.c
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-cp-details -fdump-tree-vrp1" } */
+
+static __attribute__((noinline, noclone))
+int foo (int *p)
+{
+  if (!p)
+return 0;
+  *p = 1;
+}
+
+struct st
+{
+  int a;
+  int b;
+};
+
+int bar (struct st *s)
+{
+int arr[10];
+  if (!s)
+return 0;
+  foo (>a);
+  foo ([1]);
+}
+
+/* { dg-final { scan-ipa-dump "Setting nonnull for 0" "cp" } } */
+/* { dg-final { scan-tree-dump-times "if" 1 "vrp1" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/vrp6.c b/gcc/testsuite/gcc.dg/ipa/vrp6.c
index e69de29..f837758 100644
--- a/gcc/testsuite/gcc.dg/ipa/vrp6.c
+++ b/gcc/testsuite/gcc.dg/ipa/vrp6.c
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-cp-details -fdump-tree-vrp1" } */
+
+static __attribute__((noinline, noclone))
+int foo (int *p)
+{
+  if (!p)
+return 0;
+  *p = 1;
+}
+
+struct st
+{
+  int a;
+  int b;
+};
+
+int bar (struct st *s)
+{
+struct st s2;
+  if (!s)
+return 0;
+  foo (>a);
+  foo ();
+}
+
+/* { dg-final { scan-ipa-dump "Setting nonnull for 0" "cp" } } */
+/* { dg-final { scan-tree-dump-times "if" 1 "vrp1" } } */

[patch, fortran] PR77828 Linking gfortran-7 compiled program with libgfortran of 5.x allowed but crashes

[Jerry DeLisle]

Hi Folks,

The attached patch does some minor cleanup and bumps the libgfortran version 
number.  I have wanted to reorder the dtp structure for many years now. Not 
strictly needed but it has bugged me forever.


The bump is needed because of the significant changes from implementation of 
DTIO.

I also took care of the stream I/O TODO and added a new test case for the error 
message.


On my system, I have LD_LIBRARY_PATH set to point to the new library version 
first and then the version 3 are found elsewhere. With the patch we now see the 
following behavior. (Test case in the PR)


Compile with version 6. Finds libgfortran3 on execution:
$gfc6 pr77828.f90
$ ./a.out
 Greetings from i 42 of 43

 Greetings from i 42 of 43

Compile with version 7. Finds libgfortran4 on execution:
$ gfc pr77828.f90
[jerry@amda8 pr77828]$ ./a.out
 Greetings from i 42 of 43

 Greetings from i 42 of 43

Change LD_LIBRARY_PATH to not find libgfortran4:
$ export LD_LIBRARY_PATH=/home/jerry/dev/usr6/lib64
$ gfc pr77828.f90
$ ./a.out
./a.out: error while loading shared libraries: libgfortran.so.4: cannot open 
shared object file: No such file or directory


Word of caution. When this patch is applied rebuild from a clean/empty build 
directory. You must delete any libgfortran3 remnants that may have been built 
and installed previously with gcc version 7 trunk, Otherwise the linker/loader 
may find those rather than a libgfortran3 built for gcc 6 or previous. (When 
compiling with a previous version of gcc)


New test case provided for the streamio error check.

Regression tested on x86-64-linux.

OK for trunk?

Regards,

Jerry

2016-10-18  Jerry DeLisle  

PR fortran/77828
* io/io.h (st_parameter_dt): Reorder for readability and sanity.
* io/transfer.c (data_transfer_init): Remove TODO and enable the
runtime error message, rec= specifier not allowed in STREAM
access.
* libtool-version: Bump major version of libgfortran to 4.

2016-10-18  Jerry DeLisle  

PR fortran/77828
* ioparm.def: Reorder dt parameters to match libgfortran.



diff --git a/gcc/fortran/ioparm.def b/gcc/fortran/ioparm.def
index 17b7ac78..bd628ce2 100644
--- a/gcc/fortran/ioparm.def
+++ b/gcc/fortran/ioparm.def
@@ -90,11 +90,9 @@ IOPARM (inquire, id,		1 << 7,  pint4)
 IOPARM (inquire, iqstream,	1 << 8,  char1)
 IOPARM (wait,common,	0,	 common)
 IOPARM (wait,id,		1 << 7,  pint4)
-#ifndef IOPARM_dt_list_format
+IOPARM (dt,  common,	0,	 common)
 #define IOPARM_dt_list_format		(1 << 7)
 #define IOPARM_dt_namelist_read_mode	(1 << 8)
-#endif
-IOPARM (dt,  common,	0,	 common)
 IOPARM (dt,  rec,		1 << 9,  intio)
 IOPARM (dt,  size,		1 << 10, pintio)
 IOPARM (dt,  iolength,	1 << 11, pintio)
@@ -103,7 +101,6 @@ IOPARM (dt,  format,	1 << 12, char1)
 IOPARM (dt,  advance,	1 << 13, char2)
 IOPARM (dt,  internal_unit,	1 << 14, char1)
 IOPARM (dt,  namelist_name,	1 << 15, char2)
-IOPARM (dt,  u,		0,	 pad)
 IOPARM (dt,  id,		1 << 16, pint4)
 IOPARM (dt,  pos,		1 << 17, intio)
 IOPARM (dt,  asynchronous, 	1 << 18, char1)
@@ -115,3 +112,4 @@ IOPARM (dt,  round,		1 << 23, char2)
 IOPARM (dt,  sign,		1 << 24, char1)
 #define IOPARM_dt_f2003		  (1 << 25)
 #define IOPARM_dt_dtio		  (1 << 26)
+IOPARM (dt,  u,		0,	 pad)
diff --git a/gcc/testsuite/gfortran.dg/streamio_17.f90 b/gcc/testsuite/gfortran.dg/streamio_17.f90
new file mode 100644
index ..41fa0b98
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/streamio_17.f90
@@ -0,0 +1,12 @@
+! { dg-do run }
+program stream_test
+implicit none
+integer :: ios
+character(128) :: message
+open(10, status='scratch', access='stream')
+write (10, rec=1, iostat=ios, iomsg=message) "This is a test" !
+if (ios.ne.5001) call abort
+if (message.ne. &
+  &"Record number not allowed for stream access data transfer") &
+  call abort
+end program
diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index edc520a9..cae2193b 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -424,6 +424,15 @@ typedef struct st_parameter_dt
   CHARACTER2 (advance);
   CHARACTER1 (internal_unit);
   CHARACTER2 (namelist_name);
+  GFC_INTEGER_4 *id;
+  GFC_IO_INT pos;
+  CHARACTER1 (asynchronous);
+  CHARACTER2 (blank);
+  CHARACTER1 (decimal);
+  CHARACTER2 (delim);
+  CHARACTER1 (pad);
+  CHARACTER2 (round);
+  CHARACTER1 (sign);
   /* Private part of the structure.  The compiler just needs
  to reserve enough space.  */
   union
@@ -440,7 +449,8 @@ typedef struct st_parameter_dt
 	  unit_blank blank_status;
 	  unit_sign sign_status;
 	  int scale_factor;
-	  int max_pos; /* Maximum righthand column written to.  */
+	  /* Maximum righthand column written to.  */
+	  int max_pos;
 	  /* Number of skips + spaces to be done for T and X-editing.  */
 	  int skips;
 	  /* Number of spaces to be done for T and X-editing.  */
@@ -522,15 +532,6 @@ typedef struct