Add VIEW_CONVERT_EXPR to operand_equal_p

Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

Hi,
this patch adds VIEW_CONVERT_EXPR which is another code omitted in
operand_equal_p.  During bootstrap there are about 1000 matches.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* fold-const.c (operand_equal_p): Handle VIEW_CONVERT_EXPR.
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 228735)
+++ fold-const.c	(working copy)
@@ -2962,6 +2968,12 @@ operand_equal_p (const_tree arg0, const_
 	case IMAGPART_EXPR:
 	  return OP_SAME (0);
 
+	case VIEW_CONVERT_EXPR:
+	  if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
+	      && !types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
+	    return false;
+	  return OP_SAME (0);
+
 	case TARGET_MEM_REF:
 	case MEM_REF:
 	  if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)))

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Wed, Oct 14, 2015 at 6:29 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this patch adds VIEW_CONVERT_EXPR which is another code omitted in
> operand_equal_p.  During bootstrap there are about 1000 matches.
>
> Bootstrapped/regtested x86_64-linux, OK?

Eric, does that look ok WRT TYPE_ALIGN_OK?  (that is, did we decide
TYPE_ALIGN_OK is no longer needed?)

> Honza
>
>         * fold-const.c (operand_equal_p): Handle VIEW_CONVERT_EXPR.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 228735)
> +++ fold-const.c        (working copy)
> @@ -2962,6 +2968,12 @@ operand_equal_p (const_tree arg0, const_
>         case IMAGPART_EXPR:
>           return OP_SAME (0);
>
> +       case VIEW_CONVERT_EXPR:
> +         if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
> +             && !types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))

Note that mixing the GIMPLE types_compatible_p into operand_equal_p which
is supposed to handle GENERIC as well looks dangerous.  You short-cutted
the type checks for OEP_ADDRESS_OF earlier so why do you need to
preserve them here?

The test looks bogus anyway, but maybe it's just too early in the morning ;)
Shouldn't it be && types_compatible_p (...) (instead of && !types_com...)?
Otherwise it looks really weird.

Richard.

> +           return false;
> +         return OP_SAME (0);
> +
>         case TARGET_MEM_REF:
>         case MEM_REF:
>           if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)))

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> Eric, does that look ok WRT TYPE_ALIGN_OK?  (that is, did we decide
> TYPE_ALIGN_OK is no longer needed?)

I'm not sure we need to care about TYPE_ALIGN_OK here so no objection by me.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> I'm not sure we need to care about TYPE_ALIGN_OK here so no objection by me.

Actually I have one: can we please fix the multiple Ada breakages caused by 
the previous controversial change from Jan before going farther in the series?
The build is still broken on IA-64 and I'm still seeing ICEs on x86.  Or else 
can we put the whole series on a branch until it is reasonably stable?

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> > I'm not sure we need to care about TYPE_ALIGN_OK here so no objection by me.
> 
> Actually I have one: can we please fix the multiple Ada breakages caused by 
> the previous controversial change from Jan before going farther in the series?
> The build is still broken on IA-64 and I'm still seeing ICEs on x86.  Or else 
> can we put the whole series on a branch until it is reasonably stable?

I wasn't aware that x86/IA-64 is still broken.  I am flying to NY tomorrow but will
try to take a look. The ICEs are not caused by operand_equal_p changes, but the
change to useless_type_conversion to ignore mode on aggregate types.

A safe way would be to add the mode check back (as was in my original patch)
that does not change my original intent to separate CANONICAL_TYPE from gimple
semantic type equivalence machinery. It was however outcome of the discussion that
we would preffer the mode to be ignored in this case which means fixing expansion
side.

I have no way to reproduce the IA-64 change, but will send proposed patch - from
backtrace it was clear where the wrong mode went in.  Will wait with operand_euqal_p
changess until this is fixed.

Honza
> 
> -- 
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

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

> I wasn't aware that x86/IA-64 is still broken.  I am flying to NY tomorrow
> but will try to take a look. The ICEs are not caused by operand_equal_p
> changes, but the change to useless_type_conversion to ignore mode on
> aggregate types.

Sure, but I'd like to avoid hiding new problems against preexisting ICEs.

> A safe way would be to add the mode check back (as was in my original patch)
> that does not change my original intent to separate CANONICAL_TYPE from
> gimple semantic type equivalence machinery. It was however outcome of the
> discussion that we would preffer the mode to be ignored in this case which
> means fixing expansion side.

What do we gain by doing this?  Pretending that the mode doesn't matter is a 
lie at the RTL level and I don't see why GIMPLE would have to care.

> I have no way to reproduce the IA-64 change, but will send proposed patch -
> from backtrace it was clear where the wrong mode went in.  Will wait with
> operand_euqal_p changess until this is fixed.

Thanks.  I have installed 2 testcases that exhibit 2 distinct ICEs on x86-64, 
pack21.adb at -O0 and pack22.adb at -O1 (similar to the IA-64 one).


	PR middle-end/67966
	* gnat.dg/pack21.adb: New test.
	* gnat.dg/pack22.adb: Likewise.
	* gnat.dg/pack22_pkg.ad[sb]: New helper.


-- 
Eric Botcazou

[-- Attachment #2: pack21.adb --]
[-- Type: text/x-adasrc, Size: 526 bytes --]

-- { dg-do compile }
-- { dg-options "-gnatws" }

procedure Pack21 is

  type Enum is (ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX,
                SEVEN, EIGHT, NINE, TEN, ELEVEN, TWELVE,
                THIRTEEN, FOURTEEN, FIFTEEN);

  type Rec1 is record
    I1 : INTEGER range 0 .. 800;
    I2 : INTEGER range 0 .. 15 := 0;
    E  : Enum;
  end record;
  pragma PACK (Rec1);

  type Rec2 is record
    F : Rec1;
  end record;

  for Rec2 use record
    F at 0 range 2 .. 19;
  end record;

  R1, R2 : Rec2;

begin
  null;
end;

[-- Attachment #3: pack22.adb --]
[-- Type: text/x-adasrc, Size: 371 bytes --]

-- { dg-do compile }
-- { dg-options "-O -gnatws" }

with Pack22_Pkg; use Pack22_Pkg;

procedure Pack22 is

   package Role_Map is new Bit_Map_Generic;

   type Role_List is new Role_Map.List;
   Roles_1 : Role_List;
   Roles_2 : Role_List;
   Roles_3 : Role_List;

begin
   Temp_buffer := (others => 1);
   Temp_Buffer(2) := (0);
   Roles_1 := Roles_2 xor Roles_3;
end;

[-- Attachment #4: pack22_pkg.adb --]
[-- Type: text/x-adasrc, Size: 351 bytes --]

package body Pack22_Pkg is

   package body Bit_Map_Generic is

      function "xor" (L, R : List) return List is
         Temp : List;
         for Temp'address use Temp_buffer'address;
      begin
         Temp.Bits := L.Bits xor R.Bits;
         Temp.Counter.Counter := 0;
         return Temp;
      end;

   end Bit_Map_Generic;

end Pack22_Pkg;

[-- Attachment #5: pack22_pkg.ads --]
[-- Type: text/x-adasrc, Size: 1557 bytes --]

package Pack22_Pkg is

   type byte is mod 256;
   Temp_buffer : array (0..8) of byte:= (others => 0);
   for Temp_buffer'Alignment use 2;

   subtype Id is Short_integer;

   generic
      Dummy : Integer := 0;
   package Bit_Map_Generic is

      type List is private;
      function "xor" (L, R : List) return List;

   private
      type Offset_T is range 0 .. Id'Last;
      type Counter_T is new short_integer;
      for Counter_T'Size use 16;

      type Bit_List is array (Id range <>) of Boolean;
      pragma Pack (Bit_List);

      type List_Counter_T (Is_Defined : Boolean := True) is
         record
            Dummy : Boolean := False;
            case Is_Defined is
               when True =>
                  Counter : Counter_T := 0;
               when False =>
                  null;
            end case;
         end record;
      for List_Counter_T use
         record
            Is_Defined at 0 range 0 .. 7;
            Dummy at 1 range 0 .. 7;
            Counter at 2 range 0 .. 15;
         end record;

      type List is
         record
            Offset : Offset_T := Offset_T (1) - 1;
            Counter : List_Counter_T;
            Bits : Bit_List (1 .. 6);
         end record;
      for List use
         record
            Offset at 0 range 0 .. 15;
            Counter at 2 range 0 .. 31;
         end record;

      type Iterator is
         record
            No_More_Id : Boolean := True;
            Current_Id : Id;
            The_List : List;
         end record;

   end Bit_Map_Generic;

end Pack22_Pkg;

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On October 16, 2015 5:55:08 PM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I wasn't aware that x86/IA-64 is still broken.  I am flying to NY
>tomorrow
>> but will try to take a look. The ICEs are not caused by
>operand_equal_p
>> changes, but the change to useless_type_conversion to ignore mode on
>> aggregate types.
>
>Sure, but I'd like to avoid hiding new problems against preexisting
>ICEs.
>
>> A safe way would be to add the mode check back (as was in my original
>patch)
>> that does not change my original intent to separate CANONICAL_TYPE
>from
>> gimple semantic type equivalence machinery. It was however outcome of
>the
>> discussion that we would preffer the mode to be ignored in this case
>which
>> means fixing expansion side.
>
>What do we gain by doing this?  Pretending that the mode doesn't matter
>is a 
>lie at the RTL level and I don't see why GIMPLE would have to care.

Well, it would (I think) ICE on assigning a packed variant to a non-packed variant of a strict that happens to get a non-BLKmode when not packed.

Richard.

>> I have no way to reproduce the IA-64 change, but will send proposed
>patch -
>> from backtrace it was clear where the wrong mode went in.  Will wait
>with
>> operand_euqal_p changess until this is fixed.
>
>Thanks.  I have installed 2 testcases that exhibit 2 distinct ICEs on
>x86-64, 
>pack21.adb at -O0 and pack22.adb at -O1 (similar to the IA-64 one).
>
>
>	PR middle-end/67966
>	* gnat.dg/pack21.adb: New test.
>	* gnat.dg/pack22.adb: Likewise.
>	* gnat.dg/pack22_pkg.ad[sb]: New helper.

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> Well, it would (I think) ICE on assigning a packed variant to a non-packed
> variant of a strict that happens to get a non-BLKmode when not packed.

Is "it" GIMPLE here?  My sentence was not very clear, I meant that I don't see 
why GIMPLE would have to care about whether there is a VCE or not in the IL.

And AFAIK nobody answered the question: what do we gain by making this change?
So far I have only seen breakages, suspicious fixes and code duplication...

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On October 17, 2015 11:26:43 AM GMT+02:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Well, it would (I think) ICE on assigning a packed variant to a
>non-packed
>> variant of a strict that happens to get a non-BLKmode when not
>packed.
>
>Is "it" GIMPLE here?  My sentence was not very clear, I meant that I
>don't see 
>why GIMPLE would have to care about whether there is a VCE or not in
>the IL.

Huh, I thought your question was about the mode check in useless_type_conversion_p.

>And AFAIK nobody answered the question: what do we gain by making this
>change?
>So far I have only seen breakages, suspicious fixes and code
>duplication...

Honza wants the structural equality predicate (operand_equal_p) complete (optimistically) for GIMPLE.

Richard.

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> >And AFAIK nobody answered the question: what do we gain by making this
> >change?
> >So far I have only seen breakages, suspicious fixes and code
> >duplication...
> 
> Honza wants the structural equality predicate (operand_equal_p) complete (optimistically) for GIMPLE.

There are two independent things - operand_equal_p changes and
useless_type_conversion changes.

operand_equal_p
===============

What I want to do is to merge logic of ipa-icf-gimple's
func_checker::compare_operand and operand_equal_p so we don't have two
incomplette and duplicated ways to say if two gimple operands are equal, but
one that does it right.

Main problem of func_checker::compare_operand is that it is confused about
matching types, producing too many false negatives.  Because ipa-icf works as a
propagation engine, gving up on one equivalnce leads to a cascaded effect.

operand_equal_p does handle types better, but on the other hand it does not handle
few trees that we need to match.
 - CONSTRUCTOR
 - VIEW_CONVERT_EXPR
 - OBJ_TYPE_REF

The plan is to add these into operand_equal_p and implement interface for valueizing
hook that can be used by ipa-icf-gimple to match objects cross function boundary
(for example say that two SSA_NAMEs are equal) and drop func_checker::compare_operand

usless_type_conversion
======================

I was only tracking one isse I hit: Fortran/C interoperability nees LTO to produce
same TYPE_CANONICAl for signed and unsigned version of size_t. Doing so broke
useless_type_conversion because it used TYPE_CANONICAL. We discussed the topic on
the GNU Cauldron and decided that it is cleaner to drop TYPE_CANONICAL from
useless_type_conversion because it does not really belong there.

That is only change I plan into the area. The decision to drop comparsion of TYPE_MODE
from the aggregate path was decision of the discussion about this particular patch
and I do not really insist on it.

Having fewer VCE expressions in the code is not a bad thing, but I do not really
see it as an important change. I am sorry for the breakage in move expansion that
I hoped to not be as important. I am willing to continue fixing the fallout (and
be more cureful about it - obviously I originally underestimated the issue).
I am also happy with simply adding back the mode checking and drop the changes
we did to expr.c so far.

Honza

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> I was only tracking one isse I hit: Fortran/C interoperability nees LTO to
> produce same TYPE_CANONICAl for signed and unsigned version of size_t.
> Doing so broke useless_type_conversion because it used TYPE_CANONICAL. We
> discussed the topic on the GNU Cauldron and decided that it is cleaner to
> drop TYPE_CANONICAL from useless_type_conversion because it does not really
> belong there.

OK, thanks for the explanation.

> That is only change I plan into the area. The decision to drop comparsion of
> TYPE_MODE from the aggregate path was decision of the discussion about this
> particular patch and I do not really insist on it.
> 
> Having fewer VCE expressions in the code is not a bad thing, but I do not
> really see it as an important change. I am sorry for the breakage in move
> expansion that I hoped to not be as important. I am willing to continue
> fixing the fallout (and be more cureful about it - obviously I originally
> underestimated the issue). I am also happy with simply adding back the mode
> checking and drop the changes we did to expr.c so far.

I agree on the fewer VCE expressions goal (and I have an upcoming gigi change 
to that effect) but some of them are essentially mandated by the RTL level 
and, since GENERIC & GIMPLE are ultimately lowered to RTL, they need to take 
that into account IMO.  So, if the mode change is not really necessary for the 
rest of the work, I'd restore the mode check (and this only affects Ada in 
practice since apparently only the Ada compiler fiddles with the type mode).

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

Hello,
> > I was only tracking one isse I hit: Fortran/C interoperability nees LTO to
> > produce same TYPE_CANONICAl for signed and unsigned version of size_t.
> > Doing so broke useless_type_conversion because it used TYPE_CANONICAL. We
> > discussed the topic on the GNU Cauldron and decided that it is cleaner to
> > drop TYPE_CANONICAL from useless_type_conversion because it does not really
> > belong there.
> 
> OK, thanks for the explanation.
> 
> > That is only change I plan into the area. The decision to drop comparsion of
> > TYPE_MODE from the aggregate path was decision of the discussion about this
> > particular patch and I do not really insist on it.
> > 
> > Having fewer VCE expressions in the code is not a bad thing, but I do not
> > really see it as an important change. I am sorry for the breakage in move
> > expansion that I hoped to not be as important. I am willing to continue
> > fixing the fallout (and be more cureful about it - obviously I originally
> > underestimated the issue). I am also happy with simply adding back the mode
> > checking and drop the changes we did to expr.c so far.
> 
> I agree on the fewer VCE expressions goal (and I have an upcoming gigi change 
> to that effect) but some of them are essentially mandated by the RTL level 
> and, since GENERIC & GIMPLE are ultimately lowered to RTL, they need to take 
> that into account IMO.  So, if the mode change is not really necessary for the 
> rest of the work, I'd restore the mode check (and this only affects Ada in 
> practice since apparently only the Ada compiler fiddles with the type mode).

Why is Ada fliddling with the modes? Is it only for packed structures?

I was wondering how to produce VCE convesions of aggregates with C frontend at
all (that is getting them synthetized by the middle-end) to get non-ada
testcases.  Storing through union is never folded to one and I don't see any
other obvious way of getting them.  Perhaps it may be possible to get them via
inliner on incompatible parameter and LTO, but that seems to be the only case
I can think of right now.

I am testing the change to compare modes and revert the two expr.c changes.
Lets see what is Richard's opinion. The whole concept of modes on aggregate
types is bit funny post-tree-ssa days when we do SRA. I suppose they may be
tied to calling conventions, but should no longer be needed for code quality?

Honza
> 
> -- 
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On October 18, 2015 6:06:51 PM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote:
>Hello,
>> > I was only tracking one isse I hit: Fortran/C interoperability nees
>LTO to
>> > produce same TYPE_CANONICAl for signed and unsigned version of
>size_t.
>> > Doing so broke useless_type_conversion because it used
>TYPE_CANONICAL. We
>> > discussed the topic on the GNU Cauldron and decided that it is
>cleaner to
>> > drop TYPE_CANONICAL from useless_type_conversion because it does
>not really
>> > belong there.
>> 
>> OK, thanks for the explanation.
>> 
>> > That is only change I plan into the area. The decision to drop
>comparsion of
>> > TYPE_MODE from the aggregate path was decision of the discussion
>about this
>> > particular patch and I do not really insist on it.
>> > 
>> > Having fewer VCE expressions in the code is not a bad thing, but I
>do not
>> > really see it as an important change. I am sorry for the breakage
>in move
>> > expansion that I hoped to not be as important. I am willing to
>continue
>> > fixing the fallout (and be more cureful about it - obviously I
>originally
>> > underestimated the issue). I am also happy with simply adding back
>the mode
>> > checking and drop the changes we did to expr.c so far.
>> 
>> I agree on the fewer VCE expressions goal (and I have an upcoming
>gigi change 
>> to that effect) but some of them are essentially mandated by the RTL
>level 
>> and, since GENERIC & GIMPLE are ultimately lowered to RTL, they need
>to take 
>> that into account IMO.  So, if the mode change is not really
>necessary for the 
>> rest of the work, I'd restore the mode check (and this only affects
>Ada in 
>> practice since apparently only the Ada compiler fiddles with the type
>mode).
>
>Why is Ada fliddling with the modes? Is it only for packed structures?
>
>I was wondering how to produce VCE convesions of aggregates with C
>frontend at
>all (that is getting them synthetized by the middle-end) to get non-ada
>testcases.  Storing through union is never folded to one and I don't
>see any
>other obvious way of getting them.  Perhaps it may be possible to get
>them via
>inliner on incompatible parameter and LTO, but that seems to be the
>only case
>I can think of right now.
>
>I am testing the change to compare modes and revert the two expr.c
>changes.
>Lets see what is Richard's opinion. The whole concept of modes on
>aggregate
>types is bit funny post-tree-ssa days when we do SRA. I suppose they
>may be
>tied to calling conventions, but should no longer be needed for code
>quality?

Adding back the mode check is fine if all types with the same TYPE_CANONICAL have the same mode.  Otherwise we'd regress here.  I thought we do for

Struct x { int i; };
Typedef y x __attribute__((packed));

And then doing

X x;
Y y;
X = y;

Richard.


>Honza
>> 
>> -- 
>> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> 
> Adding back the mode check is fine if all types with the same TYPE_CANONICAL have the same mode.  Otherwise we'd regress here.  I thought we do for
> 
> Struct x { int i; };
> Typedef y x __attribute__((packed));
> 
> And then doing
> 
> X x;
> Y y;
> X = y;

Do you have any idea how to turn this into a testcase? I don't think we could
add packed attribute to typedef. Even in
gimple_canonical_types_compatible_p
  /* Can't be the same type if they have different mode.  */
  if (TYPE_MODE (t1) != TYPE_MODE (t2))
    return false;
(which IMO may be wrong WRT -mavx flags where modes of same types may be different
in different TUs)

Therefore I would say that TYPE_CANONICAL determine mode modulo the fact that
incoplete variant of a complete type will have VOIDmode instead of complete
type's mode (during non-LTO).  That is why I allow mode changes for casts from
complete to incomplete.

In longer run I think that every query to useless_type_conversion_p that
contains incomplete types is a confused query.  useless_type_conversion_p is
about operations on the value and there are no operations for incomplete type
(and function types).  I know that ipa-icf-gimple and the following code in
gimplify-stmt checks this frequently:
      /* The FEs may end up building ADDR_EXPRs early on a decl with
         an incomplete type.  Re-build ADDR_EXPRs in canonical form
         here.  */
      if (!types_compatible_p (TREE_TYPE (op0), TREE_TYPE (TREE_TYPE (expr))))
        *expr_p = build_fold_addr_expr (op0);
Taking address of incomplete type or functions, naturally, makes sense.  We may
want to check something else here, like simply
       TREE_TYPE (op0) != TREE_TYPE (TREE_TYPE (expr))
and once ipa-icf is cleanded up start sanity checking in usless_type_conversion
that we use it to force equality only on types that do have values.

We also can trip it when checking TYPE_METHOD_BASETYPE which may be incomplete.
This is in the code checking useless_type_conversion on functions that I think
are confused querries anyway - we need the ABI matcher, I am looking into that.

Honza
> 
> Richard.
> 
> 
> >Honza
> >> 
> >> -- 
> >> Eric Botcazou
> 

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Sun, Oct 18, 2015 at 7:14 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> Adding back the mode check is fine if all types with the same TYPE_CANONICAL have the same mode.  Otherwise we'd regress here.  I thought we do for
>>
>> Struct x { int i; };
>> Typedef y x __attribute__((packed));
>>
>> And then doing
>>
>> X x;
>> Y y;
>> X = y;
>
> Do you have any idea how to turn this into a testcase? I don't think we could
> add packed attribute to typedef. Even in
> gimple_canonical_types_compatible_p
>   /* Can't be the same type if they have different mode.  */
>   if (TYPE_MODE (t1) != TYPE_MODE (t2))
>     return false;
> (which IMO may be wrong WRT -mavx flags where modes of same types may be different
> in different TUs)

Ok, so the following works:

struct x { int i; };
typedef struct x y __attribute__((aligned(1)));

void foo (void)
{
  struct x X;
  y Y;
  X = Y;
}

but we use SImode for y as well even though it's alignment is just one byte ...

Not sure what happens on strict-align targets for this and not sure how this
cannot be _not_ a problem.  Consider

void bar (struct x);

and

bar (Y);

or using y *Y and X = *Y or bar (*Y).

> Therefore I would say that TYPE_CANONICAL determine mode modulo the fact that
> incoplete variant of a complete type will have VOIDmode instead of complete
> type's mode (during non-LTO).  That is why I allow mode changes for casts from
> complete to incomplete.

Incomplete have VOIDmode, right?

> In longer run I think that every query to useless_type_conversion_p that
> contains incomplete types is a confused query.  useless_type_conversion_p is
> about operations on the value and there are no operations for incomplete type
> (and function types).  I know that ipa-icf-gimple and the following code in
> gimplify-stmt checks this frequently:
>       /* The FEs may end up building ADDR_EXPRs early on a decl with
>          an incomplete type.  Re-build ADDR_EXPRs in canonical form
>          here.  */
>       if (!types_compatible_p (TREE_TYPE (op0), TREE_TYPE (TREE_TYPE (expr))))
>         *expr_p = build_fold_addr_expr (op0);
> Taking address of incomplete type or functions, naturally, makes sense.  We may
> want to check something else here, like simply
>        TREE_TYPE (op0) != TREE_TYPE (TREE_TYPE (expr))
> and once ipa-icf is cleanded up start sanity checking in usless_type_conversion
> that we use it to force equality only on types that do have values.
>
> We also can trip it when checking TYPE_METHOD_BASETYPE which may be incomplete.
> This is in the code checking useless_type_conversion on functions that I think
> are confused querries anyway - we need the ABI matcher, I am looking into that.

Ok, so given we seem to be fine in practive with TYPE_MODE (type) ==
TYPE_MODE (TYPE_CANONICAL (type))
(whether that's a but or not ...) I'm fine with re-instantiating the
mode check for
aggregate types.  Please do that with

Index: gcc/gimple-expr.c
===================================================================
--- gcc/gimple-expr.c   (revision 228963)
+++ gcc/gimple-expr.c   (working copy)
@@ -89,8 +89,7 @@ useless_type_conversion_p (tree outer_ty

   /* Changes in machine mode are never useless conversions unless we
      deal with aggregate types in which case we defer to later checks.  */
-  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
-      && !AGGREGATE_TYPE_P (inner_type))
+  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type))
     return false;

   /* If both the inner and outer types are integral types, then the

Can we asses equal sizes when modes are non-BLKmode then?  Thus

@@ -270,10 +269,9 @@ useless_type_conversion_p (tree outer_ty
      use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
           && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return (!TYPE_SIZE (outer_type)
-           || (TYPE_SIZE (inner_type)
-               && operand_equal_p (TYPE_SIZE (inner_type),
-                                   TYPE_SIZE (outer_type), 0)));
+    return (TYPE_MODE (outer_type) != BLKmode
+           || operand_equal_p (TYPE_SIZE (inner_type),
+                               TYPE_SIZE (outer_type), 0));

   else if (TREE_CODE (inner_type) == OFFSET_TYPE
           && TREE_CODE (outer_type) == OFFSET_TYPE)

?  Hoping for VOIDmode incomplete case.

Richard.

> Honza
>>
>> Richard.
>>
>>
>> >Honza
>> >>
>> >> --
>> >> Eric Botcazou
>>

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

Richard,
I missed your reply earlier today.
> > Therefore I would say that TYPE_CANONICAL determine mode modulo the fact that
> > incoplete variant of a complete type will have VOIDmode instead of complete
> > type's mode (during non-LTO).  That is why I allow mode changes for casts from
> > complete to incomplete.
> 
> Incomplete have VOIDmode, right?

Yes
> 
> > In longer run I think that every query to useless_type_conversion_p that
> > contains incomplete types is a confused query.  useless_type_conversion_p is
> > about operations on the value and there are no operations for incomplete type
> > (and function types).  I know that ipa-icf-gimple and the following code in
> > gimplify-stmt checks this frequently:
> >       /* The FEs may end up building ADDR_EXPRs early on a decl with
> >          an incomplete type.  Re-build ADDR_EXPRs in canonical form
> >          here.  */
> >       if (!types_compatible_p (TREE_TYPE (op0), TREE_TYPE (TREE_TYPE (expr))))
> >         *expr_p = build_fold_addr_expr (op0);
> > Taking address of incomplete type or functions, naturally, makes sense.  We may
> > want to check something else here, like simply
> >        TREE_TYPE (op0) != TREE_TYPE (TREE_TYPE (expr))
> > and once ipa-icf is cleanded up start sanity checking in usless_type_conversion
> > that we use it to force equality only on types that do have values.
> >
> > We also can trip it when checking TYPE_METHOD_BASETYPE which may be incomplete.
> > This is in the code checking useless_type_conversion on functions that I think
> > are confused querries anyway - we need the ABI matcher, I am looking into that.
> 
> Ok, so given we seem to be fine in practive with TYPE_MODE (type) ==
> TYPE_MODE (TYPE_CANONICAL (type))

Witht the exception of incopmlete variants a type. Then TYPE_CANONICAL may
be complete and !VOIDmode.  
But sure, i believe we ought to chase away the calls to useless_type_conversion
when one of types in incomplete.
> (whether that's a but or not ...) I'm fine with re-instantiating the
> mode check for
> aggregate types.  Please do that with
> 
> Index: gcc/gimple-expr.c
> ===================================================================
> --- gcc/gimple-expr.c   (revision 228963)
> +++ gcc/gimple-expr.c   (working copy)
> @@ -89,8 +89,7 @@ useless_type_conversion_p (tree outer_ty
> 
>    /* Changes in machine mode are never useless conversions unless we
>       deal with aggregate types in which case we defer to later checks.  */
> -  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
> -      && !AGGREGATE_TYPE_P (inner_type))
> +  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type))
>      return false;

OK, that is variant of the patch I had at beggining.  I will test it.
> 
>    /* If both the inner and outer types are integral types, then the
> 
> Can we asses equal sizes when modes are non-BLKmode then?  Thus
> 
> @@ -270,10 +269,9 @@ useless_type_conversion_p (tree outer_ty
>       use the types in move operations.  */
>    else if (AGGREGATE_TYPE_P (inner_type)
>            && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> -    return (!TYPE_SIZE (outer_type)
> -           || (TYPE_SIZE (inner_type)
> -               && operand_equal_p (TYPE_SIZE (inner_type),
> -                                   TYPE_SIZE (outer_type), 0)));
> +    return (TYPE_MODE (outer_type) != BLKmode
> +           || operand_equal_p (TYPE_SIZE (inner_type),
> +                               TYPE_SIZE (outer_type), 0));
> 
>    else if (TREE_CODE (inner_type) == OFFSET_TYPE
>            && TREE_CODE (outer_type) == OFFSET_TYPE)
> 
> ?  Hoping for VOIDmode incomplete case.
Don't see why this would be a problem either.  I am going to start the testing of this variant.

Honza
> 
> Richard.
> 
> > Honza
> >>
> >> Richard.
> >>
> >>
> >> >Honza
> >> >>
> >> >> --
> >> >> Eric Botcazou
> >>

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> Adding back the mode check is fine if all types with the same TYPE_CANONICAL
> have the same mode.  Otherwise we'd regress here.

It's true for the Ada compiler, the type fiddling machinery always resets it.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> Why is Ada fliddling with the modes? Is it only for packed structures?

Yes, in Ada packing or representation clauses are allowed to modify the type 
of components, so you can have e.g. a record type with size S1 and BLKmode and 
fields of this type with a packed version of this record type (with size S2<S1 
and integer mode) and assignments between the 2 views of the type done through 
VCEs.  Moreover, in Ada, packing or representation clauses are really part of 
the language and used quite a lot so you end up with extensive type fiddling.

> I was wondering how to produce VCE convesions of aggregates with C frontend
> at all (that is getting them synthetized by the middle-end) to get non-ada
> testcases.  Storing through union is never folded to one and I don't see
> any other obvious way of getting them.  Perhaps it may be possible to get
> them via inliner on incompatible parameter and LTO, but that seems to be
> the only case I can think of right now.

That makes sense, all the machinery implementing type fiddling for the Ada 
compiler is in gigi, not in stor-layout.c for example.

> I am testing the change to compare modes and revert the two expr.c changes.
> Lets see what is Richard's opinion. The whole concept of modes on aggregate
> types is bit funny post-tree-ssa days when we do SRA. I suppose they may be
> tied to calling conventions but should no longer be needed for code quality?

Ideally it should not be tied to calling conventions either, but it is known 
that some back-ends still use it for this purpose.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

Hi,
this is patch that reverts the TYPE_MODE mismatch related changes and
adds test to type checker that TYPE_MODE always match with TYPE_CANONICAL.
I have bootstrapped/regtested x86_64-linux, but unfrtunately the regtesting
had some unrelated noise (spawn failures). I am re-testing. I am on a trip
and will likely only access interenet again from Des Moines tonight.

Honza

	* tree.c (verify_type): Verify that TYPE_MODE match
	between TYPE_CANONICAL and type.
	* expr.c (store_expr_with_bounds): Revert my previous change.
	* expmed.c (store_bit_field_1): Revert prevoius change.
	* gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE
	to match for complete types.

Index: tree.c
===================================================================
--- tree.c	(revision 228933)
+++ tree.c	(working copy)
@@ -13344,6 +13344,14 @@ verify_type (const_tree t)
       error_found = true;
     }
 
+  if (COMPLETE_TYPE_P (t) && TYPE_CANONICAL (t)
+      && TYPE_MODE (t) != TYPE_MODE (TYPE_CANONICAL (t)))
+    {
+      error ("TYPE_MODE of TYPE_CANONICAL is not compatible");
+      debug_tree (ct);
+      error_found = true;
+    }
+
 
   /* Check various uses of TYPE_MINVAL.  */
   if (RECORD_OR_UNION_TYPE_P (t))
Index: expr.c
===================================================================
--- expr.c	(revision 228933)
+++ expr.c	(working copy)
@@ -5425,14 +5425,6 @@ store_expr_with_bounds (tree exp, rtx ta
     temp = convert_modes (GET_MODE (target), TYPE_MODE (TREE_TYPE (exp)),
 			  temp, TYPE_UNSIGNED (TREE_TYPE (exp)));
 
-  /* We allow move between structures of same size but different mode.
-     If source is in memory and the mode differs, simply change the memory.  */
-  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
-    {
-      gcc_assert (MEM_P (temp));
-      temp = adjust_address_nv (temp, GET_MODE (target), 0);
-    }
-
   /* If value was not generated in the target, store it there.
      Convert the value to TARGET's type first if necessary and emit the
      pending incrementations that have been queued when expanding EXP.
Index: expmed.c
===================================================================
--- expmed.c	(revision 228933)
+++ expmed.c	(working copy)
@@ -757,14 +757,6 @@ store_bit_field_1 (rtx str_rtx, unsigned
       }
   }
 
-  /* We allow move between structures of same size but different mode.
-     If source is in memory and the mode differs, simply change the memory.  */
-  if (GET_MODE (value) == BLKmode && GET_MODE (op0) != BLKmode)
-    {
-      gcc_assert (MEM_P (value));
-      value = adjust_address_nv (value, GET_MODE (op0), 0);
-    }
-
   /* Storing an lsb-aligned field in a register
      can be done with a movstrict instruction.  */
 
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 228933)
+++ gimple-expr.c	(working copy)
@@ -88,9 +88,10 @@ useless_type_conversion_p (tree outer_ty
     return true;
 
   /* Changes in machine mode are never useless conversions unless we
-     deal with aggregate types in which case we defer to later checks.  */
+     deal with complete aggregate types in which case we defer to later
+     checks.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
-      && !AGGREGATE_TYPE_P (inner_type))
+      && (!AGGREGATE_TYPE_P (inner_type) || COMPLETE_TYPE_P (outer_type)))
     return false;
 
   /* If both the inner and outer types are integral types, then the

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> this is patch that reverts the TYPE_MODE mismatch related changes and
> adds test to type checker that TYPE_MODE always match with TYPE_CANONICAL.
> I have bootstrapped/regtested x86_64-linux, but unfrtunately the regtesting
> had some unrelated noise (spawn failures). I am re-testing. I am on a trip
> and will likely only access interenet again from Des Moines tonight.

Thanks!

> 
> 	* tree.c (verify_type): Verify that TYPE_MODE match
> 	between TYPE_CANONICAL and type.
> 	* expr.c (store_expr_with_bounds): Revert my previous change.
> 	* expmed.c (store_bit_field_1): Revert prevoius change.
> 	* gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE
> 	to match for complete types.

Please add the PR middle-end/67966 reference to the ChangeLog.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> > 
> > 	* tree.c (verify_type): Verify that TYPE_MODE match
> > 	between TYPE_CANONICAL and type.
> > 	* expr.c (store_expr_with_bounds): Revert my previous change.
> > 	* expmed.c (store_bit_field_1): Revert prevoius change.
> > 	* gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE
> > 	to match for complete types.
> 
> Please add the PR middle-end/67966 reference to the ChangeLog.

Added and comitted now. 

Honza

> 
> -- 
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Andreas Schwab]

+  /* Changes in machine mode are never useless conversions unless.  */

Unless what?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

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

> Added and comitted now.

Thanks.  Now on to the wrong code issues. :-)

Up to the change, the useless_type_conversion_p predicate was relying on 
structural equivalence via the TYPE_CANONICAL check, now it only looks at the 
outermost level (size, mode).  Now some back-ends, most notably x86-64, do a 
deep structural scan to determine the calling conventions (classify_argument) 
instead of just looking at the size and the mode, so consistency dictates that 
the type of the argument and that of the parameter be structurally equivalent 
and this sometimes can only be achieved by a VCE... which is now deleted. :-(
See the call to derivedIP in the attached testcase which now fails on x86-64.

How do we get away from here?


	* gnat.dg/discr44.adb: New test.

-- 
Eric Botcazou

[-- Attachment #2: discr44.adb --]
[-- Type: text/x-adasrc, Size: 496 bytes --]

-- { dg-do run }
-- { dg-options "-gnatws" }

procedure Discr44 is

  function Ident (I : Integer) return Integer is
  begin
    return I;
  end;

  type Int is range 1 .. 10;

  type Str is array (Int range <>) of Character;

  type Parent (D1, D2 : Int; B : Boolean) is record
    S : Str (D1 .. D2);
  end record;

  type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);

  X1 : Derived (D => Int (Ident (7)));

begin
  if X1.D /= 7 then
    raise Program_Error;
  end if;
end;

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> > Added and comitted now.
> 
> Thanks.  Now on to the wrong code issues. :-)
> 
> Up to the change, the useless_type_conversion_p predicate was relying on 
> structural equivalence via the TYPE_CANONICAL check, now it only looks at the 
> outermost level (size, mode).  Now some back-ends, most notably x86-64, do a 
> deep structural scan to determine the calling conventions (classify_argument) 
> instead of just looking at the size and the mode, so consistency dictates that 
> the type of the argument and that of the parameter be structurally equivalent 
> and this sometimes can only be achieved by a VCE... which is now deleted. :-(
> See the call to derivedIP in the attached testcase which now fails on x86-64.
> 
> How do we get away from here?

Hmm, I noticed this in ipa-icf context and wrote checker that two functions are ABI
compatile (did not pushed it out yet), but of course this is nastier.

I think the problem exists before my patch with LTO - it is just matter of
doing two types which will be considered equivalent by
gimple_canonical_types_compatible_p but have different type conversion.  An
example of such type would be:

struct a {
  int a[4];
};
struct b {
  int a[4];
} __attribute__ ((__aligned__(16)));

I tried to turn this into an testcase, the problem is that I don't know of a way
to obtain VIEW_CONVERT_EXPR between the two types out of C or C++ frontend and we
don't seem to synthetize these in middle end (even in cases it would make sense).
I will try to play with it more - would be nice to have a C reproducer.

We may be safe before my patch from wrong code issues if there is no way to 
rpduce VIEW_CONVERT_EXPR between types like this in languages that support
aligned attribute.

I think the problem is generally similar to memory references - the gimple type
compatibility should not be tied to ABI details.  Probably most consistent
solution would be to extend GIMPLE_CALL to also list types of parameters and do
not rely on whatever type the operand have....

Richard, any ideas?

Honza
> 
> 
> 	* gnat.dg/discr44.adb: New test.
> 
> -- 
> Eric Botcazou

> -- { dg-do run }
> -- { dg-options "-gnatws" }
> 
> procedure Discr44 is
> 
>   function Ident (I : Integer) return Integer is
>   begin
>     return I;
>   end;
> 
>   type Int is range 1 .. 10;
> 
>   type Str is array (Int range <>) of Character;
> 
>   type Parent (D1, D2 : Int; B : Boolean) is record
>     S : Str (D1 .. D2);
>   end record;
> 
>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
> 
>   X1 : Derived (D => Int (Ident (7)));
> 
> begin
>   if X1.D /= 7 then
>     raise Program_Error;
>   end if;
> end;

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Thu, Oct 29, 2015 at 4:39 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Added and comitted now.
>>
>> Thanks.  Now on to the wrong code issues. :-)
>>
>> Up to the change, the useless_type_conversion_p predicate was relying on
>> structural equivalence via the TYPE_CANONICAL check, now it only looks at the
>> outermost level (size, mode).  Now some back-ends, most notably x86-64, do a
>> deep structural scan to determine the calling conventions (classify_argument)
>> instead of just looking at the size and the mode, so consistency dictates that
>> the type of the argument and that of the parameter be structurally equivalent
>> and this sometimes can only be achieved by a VCE... which is now deleted. :-(
>> See the call to derivedIP in the attached testcase which now fails on x86-64.
>>
>> How do we get away from here?
>
> Hmm, I noticed this in ipa-icf context and wrote checker that two functions are ABI
> compatile (did not pushed it out yet), but of course this is nastier.
>
> I think the problem exists before my patch with LTO - it is just matter of
> doing two types which will be considered equivalent by
> gimple_canonical_types_compatible_p but have different type conversion.  An
> example of such type would be:
>
> struct a {
>   int a[4];
> };
> struct b {
>   int a[4];
> } __attribute__ ((__aligned__(16)));
>
> I tried to turn this into an testcase, the problem is that I don't know of a way
> to obtain VIEW_CONVERT_EXPR between the two types out of C or C++ frontend and we
> don't seem to synthetize these in middle end (even in cases it would make sense).
> I will try to play with it more - would be nice to have a C reproducer.
>
> We may be safe before my patch from wrong code issues if there is no way to
> rpduce VIEW_CONVERT_EXPR between types like this in languages that support
> aligned attribute.
>
> I think the problem is generally similar to memory references - the gimple type
> compatibility should not be tied to ABI details.  Probably most consistent
> solution would be to extend GIMPLE_CALL to also list types of parameters and do
> not rely on whatever type the operand have....
>
> Richard, any ideas?

IMHO it was always wrong/fragile for backends to look at the actual arguments to
decide on the calling convention.  The backends should _solely_ rely on
gimple_call_fntype and its TYPE_ARG_TYPES here.

Of course then there are varargs ... (not sure if we hit this here).

But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
what exactly we gain from it (when not done on registers).

But I also don't see where we do the stripping mentioned on memory references.
The match.pd pattern doesn't apply to memory, only in the GENERIC path
which is guarded with exact type equality.  So I can't see where we end up
stripping the V_C_E.

There is one bogus case still in fold-const.c:

    case VIEW_CONVERT_EXPR:
      if (TREE_CODE (op0) == MEM_REF)
        /* ???  Bogus for aligned types.  */
        return fold_build2_loc (loc, MEM_REF, type,
                                TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));

      return NULL_TREE;

that comment is only in my local tree ... (we lose alignment info that is
on the original MEM_REF type which may be a smaller one).

Richard.

> Honza
>>
>>
>>       * gnat.dg/discr44.adb: New test.
>>
>> --
>> Eric Botcazou
>
>> -- { dg-do run }
>> -- { dg-options "-gnatws" }
>>
>> procedure Discr44 is
>>
>>   function Ident (I : Integer) return Integer is
>>   begin
>>     return I;
>>   end;
>>
>>   type Int is range 1 .. 10;
>>
>>   type Str is array (Int range <>) of Character;
>>
>>   type Parent (D1, D2 : Int; B : Boolean) is record
>>     S : Str (D1 .. D2);
>>   end record;
>>
>>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
>>
>>   X1 : Derived (D => Int (Ident (7)));
>>
>> begin
>>   if X1.D /= 7 then
>>     raise Program_Error;
>>   end if;
>> end;
>

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Thu, Oct 29, 2015 at 12:22 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Oct 29, 2015 at 4:39 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> > Added and comitted now.
>>>
>>> Thanks.  Now on to the wrong code issues. :-)
>>>
>>> Up to the change, the useless_type_conversion_p predicate was relying on
>>> structural equivalence via the TYPE_CANONICAL check, now it only looks at the
>>> outermost level (size, mode).  Now some back-ends, most notably x86-64, do a
>>> deep structural scan to determine the calling conventions (classify_argument)
>>> instead of just looking at the size and the mode, so consistency dictates that
>>> the type of the argument and that of the parameter be structurally equivalent
>>> and this sometimes can only be achieved by a VCE... which is now deleted. :-(
>>> See the call to derivedIP in the attached testcase which now fails on x86-64.
>>>
>>> How do we get away from here?
>>
>> Hmm, I noticed this in ipa-icf context and wrote checker that two functions are ABI
>> compatile (did not pushed it out yet), but of course this is nastier.
>>
>> I think the problem exists before my patch with LTO - it is just matter of
>> doing two types which will be considered equivalent by
>> gimple_canonical_types_compatible_p but have different type conversion.  An
>> example of such type would be:
>>
>> struct a {
>>   int a[4];
>> };
>> struct b {
>>   int a[4];
>> } __attribute__ ((__aligned__(16)));
>>
>> I tried to turn this into an testcase, the problem is that I don't know of a way
>> to obtain VIEW_CONVERT_EXPR between the two types out of C or C++ frontend and we
>> don't seem to synthetize these in middle end (even in cases it would make sense).
>> I will try to play with it more - would be nice to have a C reproducer.
>>
>> We may be safe before my patch from wrong code issues if there is no way to
>> rpduce VIEW_CONVERT_EXPR between types like this in languages that support
>> aligned attribute.
>>
>> I think the problem is generally similar to memory references - the gimple type
>> compatibility should not be tied to ABI details.  Probably most consistent
>> solution would be to extend GIMPLE_CALL to also list types of parameters and do
>> not rely on whatever type the operand have....
>>
>> Richard, any ideas?
>
> IMHO it was always wrong/fragile for backends to look at the actual arguments to
> decide on the calling convention.  The backends should _solely_ rely on
> gimple_call_fntype and its TYPE_ARG_TYPES here.
>
> Of course then there are varargs ... (not sure if we hit this here).
>
> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
> what exactly we gain from it (when not done on registers).
>
> But I also don't see where we do the stripping mentioned on memory references.
> The match.pd pattern doesn't apply to memory, only in the GENERIC path
> which is guarded with exact type equality.  So I can't see where we end up
> stripping the V_C_E.
>
> There is one bogus case still in fold-const.c:
>
>     case VIEW_CONVERT_EXPR:
>       if (TREE_CODE (op0) == MEM_REF)
>         /* ???  Bogus for aligned types.  */
>         return fold_build2_loc (loc, MEM_REF, type,
>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
>
>       return NULL_TREE;
>
> that comment is only in my local tree ... (we lose alignment info that is
> on the original MEM_REF type which may be a smaller one).

Ah - tree_ssa_useless_type_conversion and callers, during gimplification.
I'd like to get rid of it but maybe simply delete the VIEW_CONVERT_EXPR
case from it for now (and return true unconditionally for NON_LVALUE_EXPR).

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c      (revision 229517)
+++ gcc/tree-ssa.c      (working copy)
@@ -1142,13 +1161,16 @@ delete_tree_ssa (struct function *fn)
 bool
 tree_ssa_useless_type_conversion (tree expr)
 {
+  /* Not strictly a conversion but this function is used to strip
+     useless stuff from trees returned from GENERIC folding.  */
+  if (TREE_CODE (expr) == NON_LVALUE_EXPR)
+    return true;
+
   /* If we have an assignment that merely uses a NOP_EXPR to change
      the top of the RHS to the type of the LHS and the type conversion
      is "safe", then strip away the type conversion so that we can
      enter LHS = RHS into the const_and_copies table.  */
-  if (CONVERT_EXPR_P (expr)
-      || TREE_CODE (expr) == VIEW_CONVERT_EXPR
-      || TREE_CODE (expr) == NON_LVALUE_EXPR)
+  if (CONVERT_EXPR_P (expr))
     return useless_type_conversion_p
       (TREE_TYPE (expr),
        TREE_TYPE (TREE_OPERAND (expr, 0)));

IMHO the gimplifier use should be more explicit and most remaining GIMPLE
middle-end uses should be removed (after auditing).

Richard.

> Richard.
>
>> Honza
>>>
>>>
>>>       * gnat.dg/discr44.adb: New test.
>>>
>>> --
>>> Eric Botcazou
>>
>>> -- { dg-do run }
>>> -- { dg-options "-gnatws" }
>>>
>>> procedure Discr44 is
>>>
>>>   function Ident (I : Integer) return Integer is
>>>   begin
>>>     return I;
>>>   end;
>>>
>>>   type Int is range 1 .. 10;
>>>
>>>   type Str is array (Int range <>) of Character;
>>>
>>>   type Parent (D1, D2 : Int; B : Boolean) is record
>>>     S : Str (D1 .. D2);
>>>   end record;
>>>
>>>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
>>>
>>>   X1 : Derived (D => Int (Ident (7)));
>>>
>>> begin
>>>   if X1.D /= 7 then
>>>     raise Program_Error;
>>>   end if;
>>> end;
>>

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Thu, Oct 29, 2015 at 12:31 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Oct 29, 2015 at 12:22 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Oct 29, 2015 at 4:39 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> > Added and comitted now.
>>>>
>>>> Thanks.  Now on to the wrong code issues. :-)
>>>>
>>>> Up to the change, the useless_type_conversion_p predicate was relying on
>>>> structural equivalence via the TYPE_CANONICAL check, now it only looks at the
>>>> outermost level (size, mode).  Now some back-ends, most notably x86-64, do a
>>>> deep structural scan to determine the calling conventions (classify_argument)
>>>> instead of just looking at the size and the mode, so consistency dictates that
>>>> the type of the argument and that of the parameter be structurally equivalent
>>>> and this sometimes can only be achieved by a VCE... which is now deleted. :-(
>>>> See the call to derivedIP in the attached testcase which now fails on x86-64.
>>>>
>>>> How do we get away from here?
>>>
>>> Hmm, I noticed this in ipa-icf context and wrote checker that two functions are ABI
>>> compatile (did not pushed it out yet), but of course this is nastier.
>>>
>>> I think the problem exists before my patch with LTO - it is just matter of
>>> doing two types which will be considered equivalent by
>>> gimple_canonical_types_compatible_p but have different type conversion.  An
>>> example of such type would be:
>>>
>>> struct a {
>>>   int a[4];
>>> };
>>> struct b {
>>>   int a[4];
>>> } __attribute__ ((__aligned__(16)));
>>>
>>> I tried to turn this into an testcase, the problem is that I don't know of a way
>>> to obtain VIEW_CONVERT_EXPR between the two types out of C or C++ frontend and we
>>> don't seem to synthetize these in middle end (even in cases it would make sense).
>>> I will try to play with it more - would be nice to have a C reproducer.
>>>
>>> We may be safe before my patch from wrong code issues if there is no way to
>>> rpduce VIEW_CONVERT_EXPR between types like this in languages that support
>>> aligned attribute.
>>>
>>> I think the problem is generally similar to memory references - the gimple type
>>> compatibility should not be tied to ABI details.  Probably most consistent
>>> solution would be to extend GIMPLE_CALL to also list types of parameters and do
>>> not rely on whatever type the operand have....
>>>
>>> Richard, any ideas?
>>
>> IMHO it was always wrong/fragile for backends to look at the actual arguments to
>> decide on the calling convention.  The backends should _solely_ rely on
>> gimple_call_fntype and its TYPE_ARG_TYPES here.
>>
>> Of course then there are varargs ... (not sure if we hit this here).
>>
>> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
>> what exactly we gain from it (when not done on registers).
>>
>> But I also don't see where we do the stripping mentioned on memory references.
>> The match.pd pattern doesn't apply to memory, only in the GENERIC path
>> which is guarded with exact type equality.  So I can't see where we end up
>> stripping the V_C_E.
>>
>> There is one bogus case still in fold-const.c:
>>
>>     case VIEW_CONVERT_EXPR:
>>       if (TREE_CODE (op0) == MEM_REF)
>>         /* ???  Bogus for aligned types.  */
>>         return fold_build2_loc (loc, MEM_REF, type,
>>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
>>
>>       return NULL_TREE;
>>
>> that comment is only in my local tree ... (we lose alignment info that is
>> on the original MEM_REF type which may be a smaller one).
>
> Ah - tree_ssa_useless_type_conversion and callers, during gimplification.
> I'd like to get rid of it but maybe simply delete the VIEW_CONVERT_EXPR
> case from it for now (and return true unconditionally for NON_LVALUE_EXPR).
>
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 229517)
> +++ gcc/tree-ssa.c      (working copy)
> @@ -1142,13 +1161,16 @@ delete_tree_ssa (struct function *fn)
>  bool
>  tree_ssa_useless_type_conversion (tree expr)
>  {
> +  /* Not strictly a conversion but this function is used to strip
> +     useless stuff from trees returned from GENERIC folding.  */
> +  if (TREE_CODE (expr) == NON_LVALUE_EXPR)
> +    return true;
> +
>    /* If we have an assignment that merely uses a NOP_EXPR to change
>       the top of the RHS to the type of the LHS and the type conversion
>       is "safe", then strip away the type conversion so that we can
>       enter LHS = RHS into the const_and_copies table.  */
> -  if (CONVERT_EXPR_P (expr)
> -      || TREE_CODE (expr) == VIEW_CONVERT_EXPR
> -      || TREE_CODE (expr) == NON_LVALUE_EXPR)
> +  if (CONVERT_EXPR_P (expr))
>      return useless_type_conversion_p
>        (TREE_TYPE (expr),
>         TREE_TYPE (TREE_OPERAND (expr, 0)));
>
> IMHO the gimplifier use should be more explicit and most remaining GIMPLE
> middle-end uses should be removed (after auditing).

The above is pre-approved if one of you does the required testing (it
fixes the Ada
testcase for me).

Richard.

> Richard.
>
>> Richard.
>>
>>> Honza
>>>>
>>>>
>>>>       * gnat.dg/discr44.adb: New test.
>>>>
>>>> --
>>>> Eric Botcazou
>>>
>>>> -- { dg-do run }
>>>> -- { dg-options "-gnatws" }
>>>>
>>>> procedure Discr44 is
>>>>
>>>>   function Ident (I : Integer) return Integer is
>>>>   begin
>>>>     return I;
>>>>   end;
>>>>
>>>>   type Int is range 1 .. 10;
>>>>
>>>>   type Str is array (Int range <>) of Character;
>>>>
>>>>   type Parent (D1, D2 : Int; B : Boolean) is record
>>>>     S : Str (D1 .. D2);
>>>>   end record;
>>>>
>>>>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
>>>>
>>>>   X1 : Derived (D => Int (Ident (7)));
>>>>
>>>> begin
>>>>   if X1.D /= 7 then
>>>>     raise Program_Error;
>>>>   end if;
>>>> end;
>>>

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

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

> Ah - tree_ssa_useless_type_conversion and callers, during gimplification.
> I'd like to get rid of it but maybe simply delete the VIEW_CONVERT_EXPR
> case from it for now (and return true unconditionally for NON_LVALUE_EXPR).
> 
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 229517)
> +++ gcc/tree-ssa.c      (working copy)
> @@ -1142,13 +1161,16 @@ delete_tree_ssa (struct function *fn)
>  bool
>  tree_ssa_useless_type_conversion (tree expr)
>  {
> +  /* Not strictly a conversion but this function is used to strip
> +     useless stuff from trees returned from GENERIC folding.  */
> +  if (TREE_CODE (expr) == NON_LVALUE_EXPR)
> +    return true;
> +
>    /* If we have an assignment that merely uses a NOP_EXPR to change
>       the top of the RHS to the type of the LHS and the type conversion
>       is "safe", then strip away the type conversion so that we can
>       enter LHS = RHS into the const_and_copies table.  */
> -  if (CONVERT_EXPR_P (expr)
> -      || TREE_CODE (expr) == VIEW_CONVERT_EXPR
> -      || TREE_CODE (expr) == NON_LVALUE_EXPR)
> +  if (CONVERT_EXPR_P (expr))
>      return useless_type_conversion_p
>        (TREE_TYPE (expr),
>         TREE_TYPE (TREE_OPERAND (expr, 0)));

The patch introduces GIMPLE checking failures:

FAIL:   c52103m
FAIL:   c52103r
FAIL:   c52104m
FAIL:   c52104r

of the form:

slice9.adb: In function 'Slice9':
slice9.adb:1:1: error: conversion of an SSA_NAME on the left hand side
VIEW_CONVERT_EXPR<character[D.4195:iftmp.10]>("ABCDE");

D.4379 = &VIEW_CONVERT_EXPR<character[D.4195:iftmp.10]>("ABCDE")[D.4378 ...]
{lb: D.4195 sz: 1};

      if (TREE_CODE (expr) == VIEW_CONVERT_EXPR)
	{
	  /* For VIEW_CONVERT_EXPRs which are allowed here too, we only check
	     that their operand is not an SSA name or an invariant when
	     requiring an lvalue (this usually means there is a SRA or IPA-SRA
	     bug).  Otherwise there is nothing to verify, gross mismatches at
	     most invoke undefined behavior.  */
	  if (require_lvalue
	      && (TREE_CODE (op) == SSA_NAME
		  || is_gimple_min_invariant (op)))
	    {
	      error ("conversion of an SSA_NAME on the left hand side");
	      debug_generic_stmt (expr);
	      return true;
	    }
	  else if (TREE_CODE (op) == SSA_NAME
		&& TYPE_SIZE (TREE_TYPE (expr)) != TYPE_SIZE (TREE_TYPE (op)))
	    {
	      error ("conversion of register to a different size");
	      debug_generic_stmt (expr);
	      return true;
	    }
	  else if (!handled_component_p (op))
	    return false;
	}

It's related to dynamic slicing (reduced testcase attached).


	* gnat.dg/slice9.adb: New test.

-- 
Eric Botcazou

[-- Attachment #2: slice9.adb --]
[-- Type: text/x-adasrc, Size: 297 bytes --]

-- { dg-do compile }

procedure Slice9 is

  function Ident (I : Integer) return Integer is
  begin
    return I;
  end;

  subtype S is String (Ident(5)..Ident(9));

  Dest : S;

  Src : String (Ident(1)..Ident(5)) := "ABCDE";

begin
  Dest (Ident(5)..Ident(7)) := Src (Ident(1)..Ident(3));
end;

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> 
> IMHO it was always wrong/fragile for backends to look at the actual arguments to
> decide on the calling convention.  The backends should _solely_ rely on
> gimple_call_fntype and its TYPE_ARG_TYPES here.
> 
> Of course then there are varargs ... (not sure if we hit this here).

Yep, you have varargs and K&R prototypes, so it can't work this way.
> 
> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
> what exactly we gain from it (when not done on registers).

I guess gain is really limited to Ada - there are very few cases we do VCE otherwise.
(I think we could do more of them).  We can make useless_type_conversion NOP/CONVERT
only. That in fact makes quite a sense because those are types with gimple operations
on it.  Perhaps also VCE on vectors, but not VCE in general.

Honza
> 
> But I also don't see where we do the stripping mentioned on memory references.
> The match.pd pattern doesn't apply to memory, only in the GENERIC path
> which is guarded with exact type equality.  So I can't see where we end up
> stripping the V_C_E.
> 
> There is one bogus case still in fold-const.c:
> 
>     case VIEW_CONVERT_EXPR:
>       if (TREE_CODE (op0) == MEM_REF)
>         /* ???  Bogus for aligned types.  */
>         return fold_build2_loc (loc, MEM_REF, type,
>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
> 
>       return NULL_TREE;
> 
> that comment is only in my local tree ... (we lose alignment info that is
> on the original MEM_REF type which may be a smaller one).
> 
> Richard.
> 
> > Honza
> >>
> >>
> >>       * gnat.dg/discr44.adb: New test.
> >>
> >> --
> >> Eric Botcazou
> >
> >> -- { dg-do run }
> >> -- { dg-options "-gnatws" }
> >>
> >> procedure Discr44 is
> >>
> >>   function Ident (I : Integer) return Integer is
> >>   begin
> >>     return I;
> >>   end;
> >>
> >>   type Int is range 1 .. 10;
> >>
> >>   type Str is array (Int range <>) of Character;
> >>
> >>   type Parent (D1, D2 : Int; B : Boolean) is record
> >>     S : Str (D1 .. D2);
> >>   end record;
> >>
> >>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
> >>
> >>   X1 : Derived (D => Int (Ident (7)));
> >>
> >> begin
> >>   if X1.D /= 7 then
> >>     raise Program_Error;
> >>   end if;
> >> end;
> >

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Thu, Oct 29, 2015 at 4:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> IMHO it was always wrong/fragile for backends to look at the actual arguments to
>> decide on the calling convention.  The backends should _solely_ rely on
>> gimple_call_fntype and its TYPE_ARG_TYPES here.
>>
>> Of course then there are varargs ... (not sure if we hit this here).
>
> Yep, you have varargs and K&R prototypes, so it can't work this way.

Well, then I suppose we need to compute the ABI upfront when we gimplify
from the orginal args (like we preserve fntype).  Having a separate fntype
was really meant to make us preserve the ABI throughout the GIMPLE phase...

>>
>> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
>> what exactly we gain from it (when not done on registers).
>
> I guess gain is really limited to Ada - there are very few cases we do VCE otherwise.
> (I think we could do more of them).  We can make useless_type_conversion NOP/CONVERT
> only. That in fact makes quite a sense because those are types with gimple operations
> on it.  Perhaps also VCE on vectors, but not VCE in general.
>
> Honza
>>
>> But I also don't see where we do the stripping mentioned on memory references.
>> The match.pd pattern doesn't apply to memory, only in the GENERIC path
>> which is guarded with exact type equality.  So I can't see where we end up
>> stripping the V_C_E.
>>
>> There is one bogus case still in fold-const.c:
>>
>>     case VIEW_CONVERT_EXPR:
>>       if (TREE_CODE (op0) == MEM_REF)
>>         /* ???  Bogus for aligned types.  */
>>         return fold_build2_loc (loc, MEM_REF, type,
>>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
>>
>>       return NULL_TREE;
>>
>> that comment is only in my local tree ... (we lose alignment info that is
>> on the original MEM_REF type which may be a smaller one).
>>
>> Richard.
>>
>> > Honza
>> >>
>> >>
>> >>       * gnat.dg/discr44.adb: New test.
>> >>
>> >> --
>> >> Eric Botcazou
>> >
>> >> -- { dg-do run }
>> >> -- { dg-options "-gnatws" }
>> >>
>> >> procedure Discr44 is
>> >>
>> >>   function Ident (I : Integer) return Integer is
>> >>   begin
>> >>     return I;
>> >>   end;
>> >>
>> >>   type Int is range 1 .. 10;
>> >>
>> >>   type Str is array (Int range <>) of Character;
>> >>
>> >>   type Parent (D1, D2 : Int; B : Boolean) is record
>> >>     S : Str (D1 .. D2);
>> >>   end record;
>> >>
>> >>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
>> >>
>> >>   X1 : Derived (D => Int (Ident (7)));
>> >>
>> >> begin
>> >>   if X1.D /= 7 then
>> >>     raise Program_Error;
>> >>   end if;
>> >> end;
>> >

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> On Thu, Oct 29, 2015 at 4:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >>
> >> IMHO it was always wrong/fragile for backends to look at the actual arguments to
> >> decide on the calling convention.  The backends should _solely_ rely on
> >> gimple_call_fntype and its TYPE_ARG_TYPES here.
> >>
> >> Of course then there are varargs ... (not sure if we hit this here).
> >
> > Yep, you have varargs and K&R prototypes, so it can't work this way.
> 
> Well, then I suppose we need to compute the ABI upfront when we gimplify
> from the orginal args (like we preserve fntype).  Having a separate fntype
> was really meant to make us preserve the ABI throughout the GIMPLE phase...

Hmm, the idea of doing some part of ABI explicitly is definitly nice (at least
the implicit promotions and pass by reference bits), but storing the full
lowlevel info on how to pass argument seems bit steep. You will need to
preserve the RTL containers for parameters that may get non-trivial (PARALLEL)
and precompute all the other information how to get data on stack. 

While playing with the ABi checker I was just looking into this after several
years (when i was cleaning up calls.c) and calls.c basically works by computing
arg_data that holds most of the info you would need (you need also return
argument passing and the hidden argument for structure returns).  You can check
it out - it is fairly non-trivial beast plus it really holds two parallel sets
of infos - tailcall and normal call (because these differ for targets with
register windows). The info also depends on flags used to compile function body
(such as -maccumulate-outgoing-args)

To make something like this a permanent part of GIMPLE would probably need quite
careful re-engineering of the APIs inventing more high-level intermediate
representation to get out of the machine description.  There is not realy immediate
benefit from knowing how parameters are housed on stack for gimple optimizers, so
perhaps just keeping the type information (after promotions) as the way to specify
call conventions is more practical way to go.

Honza

> >> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
> >> what exactly we gain from it (when not done on registers).
> >
> > I guess gain is really limited to Ada - there are very few cases we do VCE otherwise.
> > (I think we could do more of them).  We can make useless_type_conversion NOP/CONVERT
> > only. That in fact makes quite a sense because those are types with gimple operations
> > on it.  Perhaps also VCE on vectors, but not VCE in general.
> >
> > Honza
> >>
> >> But I also don't see where we do the stripping mentioned on memory references.
> >> The match.pd pattern doesn't apply to memory, only in the GENERIC path
> >> which is guarded with exact type equality.  So I can't see where we end up
> >> stripping the V_C_E.
> >>
> >> There is one bogus case still in fold-const.c:
> >>
> >>     case VIEW_CONVERT_EXPR:
> >>       if (TREE_CODE (op0) == MEM_REF)
> >>         /* ???  Bogus for aligned types.  */
> >>         return fold_build2_loc (loc, MEM_REF, type,
> >>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
> >>
> >>       return NULL_TREE;
> >>
> >> that comment is only in my local tree ... (we lose alignment info that is
> >> on the original MEM_REF type which may be a smaller one).
> >>
> >> Richard.
> >>
> >> > Honza
> >> >>
> >> >>
> >> >>       * gnat.dg/discr44.adb: New test.
> >> >>
> >> >> --
> >> >> Eric Botcazou
> >> >
> >> >> -- { dg-do run }
> >> >> -- { dg-options "-gnatws" }
> >> >>
> >> >> procedure Discr44 is
> >> >>
> >> >>   function Ident (I : Integer) return Integer is
> >> >>   begin
> >> >>     return I;
> >> >>   end;
> >> >>
> >> >>   type Int is range 1 .. 10;
> >> >>
> >> >>   type Str is array (Int range <>) of Character;
> >> >>
> >> >>   type Parent (D1, D2 : Int; B : Boolean) is record
> >> >>     S : Str (D1 .. D2);
> >> >>   end record;
> >> >>
> >> >>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
> >> >>
> >> >>   X1 : Derived (D => Int (Ident (7)));
> >> >>
> >> >> begin
> >> >>   if X1.D /= 7 then
> >> >>     raise Program_Error;
> >> >>   end if;
> >> >> end;
> >> >

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Thu, Oct 29, 2015 at 4:52 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Thu, Oct 29, 2015 at 4:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >>
>> >> IMHO it was always wrong/fragile for backends to look at the actual arguments to
>> >> decide on the calling convention.  The backends should _solely_ rely on
>> >> gimple_call_fntype and its TYPE_ARG_TYPES here.
>> >>
>> >> Of course then there are varargs ... (not sure if we hit this here).
>> >
>> > Yep, you have varargs and K&R prototypes, so it can't work this way.
>>
>> Well, then I suppose we need to compute the ABI upfront when we gimplify
>> from the orginal args (like we preserve fntype).  Having a separate fntype
>> was really meant to make us preserve the ABI throughout the GIMPLE phase...
>
> Hmm, the idea of doing some part of ABI explicitly is definitly nice (at least
> the implicit promotions and pass by reference bits), but storing the full
> lowlevel info on how to pass argument seems bit steep. You will need to
> preserve the RTL containers for parameters that may get non-trivial (PARALLEL)
> and precompute all the other information how to get data on stack.
>
> While playing with the ABi checker I was just looking into this after several
> years (when i was cleaning up calls.c) and calls.c basically works by computing
> arg_data that holds most of the info you would need (you need also return
> argument passing and the hidden argument for structure returns).  You can check
> it out - it is fairly non-trivial beast plus it really holds two parallel sets
> of infos - tailcall and normal call (because these differ for targets with
> register windows). The info also depends on flags used to compile function body
> (such as -maccumulate-outgoing-args)
>
> To make something like this a permanent part of GIMPLE would probably need quite
> careful re-engineering of the APIs inventing more high-level intermediate
> representation to get out of the machine description.  There is not realy immediate
> benefit from knowing how parameters are housed on stack for gimple optimizers, so
> perhaps just keeping the type information (after promotions) as the way to specify
> call conventions is more practical way to go.

Yeah, I suppose we'd need to either build a new function type for each
variadic call
then or somehow represent 'fntype' differently (note that function
attributes also
need to be preserved).

Richard.

> Honza
>
>> >> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
>> >> what exactly we gain from it (when not done on registers).
>> >
>> > I guess gain is really limited to Ada - there are very few cases we do VCE otherwise.
>> > (I think we could do more of them).  We can make useless_type_conversion NOP/CONVERT
>> > only. That in fact makes quite a sense because those are types with gimple operations
>> > on it.  Perhaps also VCE on vectors, but not VCE in general.
>> >
>> > Honza
>> >>
>> >> But I also don't see where we do the stripping mentioned on memory references.
>> >> The match.pd pattern doesn't apply to memory, only in the GENERIC path
>> >> which is guarded with exact type equality.  So I can't see where we end up
>> >> stripping the V_C_E.
>> >>
>> >> There is one bogus case still in fold-const.c:
>> >>
>> >>     case VIEW_CONVERT_EXPR:
>> >>       if (TREE_CODE (op0) == MEM_REF)
>> >>         /* ???  Bogus for aligned types.  */
>> >>         return fold_build2_loc (loc, MEM_REF, type,
>> >>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
>> >>
>> >>       return NULL_TREE;
>> >>
>> >> that comment is only in my local tree ... (we lose alignment info that is
>> >> on the original MEM_REF type which may be a smaller one).
>> >>
>> >> Richard.
>> >>
>> >> > Honza
>> >> >>
>> >> >>
>> >> >>       * gnat.dg/discr44.adb: New test.
>> >> >>
>> >> >> --
>> >> >> Eric Botcazou
>> >> >
>> >> >> -- { dg-do run }
>> >> >> -- { dg-options "-gnatws" }
>> >> >>
>> >> >> procedure Discr44 is
>> >> >>
>> >> >>   function Ident (I : Integer) return Integer is
>> >> >>   begin
>> >> >>     return I;
>> >> >>   end;
>> >> >>
>> >> >>   type Int is range 1 .. 10;
>> >> >>
>> >> >>   type Str is array (Int range <>) of Character;
>> >> >>
>> >> >>   type Parent (D1, D2 : Int; B : Boolean) is record
>> >> >>     S : Str (D1 .. D2);
>> >> >>   end record;
>> >> >>
>> >> >>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
>> >> >>
>> >> >>   X1 : Derived (D => Int (Ident (7)));
>> >> >>
>> >> >> begin
>> >> >>   if X1.D /= 7 then
>> >> >>     raise Program_Error;
>> >> >>   end if;
>> >> >> end;
>> >> >

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> 
> Yeah, I suppose we'd need to either build a new function type for each
> variadic call
> then or somehow represent 'fntype' differently (note that function
> attributes also
> need to be preserved).

Why we can't keep fntype as it is, but simply add a new set of parameters to call stmt
that lists their types?  We can then feed those types to expand_call (since we still go
back to generic here I suppose we will just re-insert those VCEs in expand-cfg.c)

Honza

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Fri, Oct 30, 2015 at 4:19 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> Yeah, I suppose we'd need to either build a new function type for each
>> variadic call
>> then or somehow represent 'fntype' differently (note that function
>> attributes also
>> need to be preserved).
>
> Why we can't keep fntype as it is, but simply add a new set of parameters to call stmt
> that lists their types?  We can then feed those types to expand_call (since we still go
> back to generic here I suppose we will just re-insert those VCEs in expand-cfg.c)

That would be quite expensive though ... (the extra list of parameters).

> Honza

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> > But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't
> > remember what exactly we gain from it (when not done on registers).
> 
> I guess gain is really limited to Ada - there are very few cases we do VCE
> otherwise. (I think we could do more of them).  We can make
> useless_type_conversion NOP/CONVERT only. That in fact makes quite a sense
> because those are types with gimple operations on it.  Perhaps also VCE on
> vectors, but not VCE in general.

FWIW that's fine with me.  Yes, Ada tends to generate a lot of VCEs but I try 
to get rid of the useless ones as much as I can so assistance from the middle-
end is not really required.  I'll test Richard's patch and install it if the 
outcome is positive (unless you want to do the vector thing right away).

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> > > But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't
> > > remember what exactly we gain from it (when not done on registers).
> > 
> > I guess gain is really limited to Ada - there are very few cases we do VCE
> > otherwise. (I think we could do more of them).  We can make
> > useless_type_conversion NOP/CONVERT only. That in fact makes quite a sense
> > because those are types with gimple operations on it.  Perhaps also VCE on
> > vectors, but not VCE in general.
> 
> FWIW that's fine with me.  Yes, Ada tends to generate a lot of VCEs but I try 
> to get rid of the useless ones as much as I can so assistance from the middle-
> end is not really required.  I'll test Richard's patch and install it if the 
> outcome is positive (unless you want to do the vector thing right away).

Lets go with this patch and hopefully stabilize the tree.  I don't think the vector
conversions represent an important case.

Honza
> 
> -- 
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

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

> Lets go with this patch and hopefully stabilize the tree.  I don't think the
> vector conversions represent an important case.

Unfortunately the patch introduces GIMPLE checking failures in Ada so it will 
need to be completed/improved.  But let's postpone it because we have another 
class of GIMPLE checking failures introduced by the useless_type_conversion_p 
change itself:

c37213j.adb:21:05: warning: variable "X" is read but never assigned
c37213j.adb: In function 'C37213J.PROC.CONSTPROP':
c37213j.adb:41:4: error: invalid conversion in gimple call
struct c37213j__proc__value___PAD

struct c37213j__proc__value___PAD

# .MEM_38 = VDEF <.MEM_37>
MEM[(struct c37213j__proc__value___PAD *)R.12_25] = c37213j.proc.value (); 
[static-chain: &FRAME.39] [return slot optimization]

and:

eric@polaris:~/build/gcc/native> ~/install/gnat-head/bin/gcc -S c37213j.adb -
O2
c37213j.adb:21:05: warning: variable "X" is read but never assigned
c37213j.adb: In function 'C37213J.PROC.VALUE':
c37213j.adb:26:5: error: invalid conversion in return statement
struct c37213j__proc__value___PAD

struct c37213j__proc__value___PAD

# VUSE <.MEM_11>
return _9(D);

What happens here is that GIMPLE statements are remapped through cloning and 
we have a variably-modified type returned by a nested function, so the type of 
the LHS of a GIMPLE_CALL or that of the RHS of a GIMPLE_RETURN is remapped but 
of course not the return type of the function.  This used to be OK because 
remapping is done by means of copy_node and preserves TYPE_CANONICAL, so the 
conversion between remapped and original type was deemed useless; now the 
TYPE_CANONICAL check is gone so the conversion is not useless anymore...

I don't think that we want to introduce an artificial VCE to fix this so we 
probably need a couple of kludges in the GIMPLE verifier instead.

In any case, the more I look into the fallout of the useless_type_conversion_p 
change, the more I find it ill-advised.  We used to have a solid type system 
in the middle-end by means of the predicate and now we have cases for which it 
ought to return false and returns true (e.g. non-structurally equivalent types 
with different calling conventions) and cases for which it can return true and 
returns false (remapped types or types deemed equivalent by the languages).
I don't really know what it was made for, but there must be a better way...


	* gnat.dg/discr45.adb: New test.

-- 
Eric Botcazou

[-- Attachment #2: discr45.adb --]
[-- Type: text/x-adasrc, Size: 806 bytes --]

-- { dg-do run }
-- { dg-options "-O2 -gnatws" }

procedure Discr45 is

  function Ident_Int (I : Integer) return Integer is
  begin
    return I;
  end;

  procedure Proc (Signal : Boolean) is

    subtype Index is Integer range 1..10;

    type My_Arr is array (Index range <>) OF Integer;

    type Rec (D3 : Integer := Ident_Int(1)) is record
      case D3 is
        when -5..10 => C1 : My_Arr(D3..Ident_Int(11));
        when Others => C2 : Integer := Ident_Int(5);
      end case;
    end record;

    X : Rec;

    function Value return Rec;
    pragma No_Inline (Value);

    function Value return Rec is
    begin
      return X;
    end;

  begin
    if X /= Value then
      raise Constraint_Error;
    elsif Signal then
      raise Program_Error;
    end if;
  end;

begin
  Proc (True);
end;

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On October 31, 2015 6:17:35 PM GMT+01:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Lets go with this patch and hopefully stabilize the tree.  I don't
>think the
>> vector conversions represent an important case.
>
>Unfortunately the patch introduces GIMPLE checking failures in Ada so
>it will 
>need to be completed/improved.  But let's postpone it because we have
>another 
>class of GIMPLE checking failures introduced by the
>useless_type_conversion_p 
>change itself:
>
>c37213j.adb:21:05: warning: variable "X" is read but never assigned
>c37213j.adb: In function 'C37213J.PROC.CONSTPROP':
>c37213j.adb:41:4: error: invalid conversion in gimple call
>struct c37213j__proc__value___PAD
>
>struct c37213j__proc__value___PAD
>
># .MEM_38 = VDEF <.MEM_37>
>MEM[(struct c37213j__proc__value___PAD *)R.12_25] = c37213j.proc.value
>(); 
>[static-chain: &FRAME.39] [return slot optimization]
>
>and:
>
>eric@polaris:~/build/gcc/native> ~/install/gnat-head/bin/gcc -S
>c37213j.adb -
>O2
>c37213j.adb:21:05: warning: variable "X" is read but never assigned
>c37213j.adb: In function 'C37213J.PROC.VALUE':
>c37213j.adb:26:5: error: invalid conversion in return statement
>struct c37213j__proc__value___PAD
>
>struct c37213j__proc__value___PAD
>
># VUSE <.MEM_11>
>return _9(D);
>
>What happens here is that GIMPLE statements are remapped through
>cloning and 
>we have a variably-modified type returned by a nested function, so the
>type of 
>the LHS of a GIMPLE_CALL or that of the RHS of a GIMPLE_RETURN is
>remapped but 
>of course not the return type of the function.  This used to be OK
>because 
>remapping is done by means of copy_node and preserves TYPE_CANONICAL,
>so the 
>conversion between remapped and original type was deemed useless; now
>the 
>TYPE_CANONICAL check is gone so the conversion is not useless
>anymore...
>
>I don't think that we want to introduce an artificial VCE to fix this
>so we 
>probably need a couple of kludges in the GIMPLE verifier instead.
>
>In any case, the more I look into the fallout of the
>useless_type_conversion_p 
>change, the more I find it ill-advised.  We used to have a solid type
>system 
>in the middle-end by means of the predicate and now we have cases for
>which it 
>ought to return false and returns true (e.g. non-structurally
>equivalent types 
>with different calling conventions) and cases for which it can return
>true and 
>returns false (remapped types or types deemed equivalent by the
>languages).
>I don't really know what it was made for, but there must be a better
>way...

I suggest to re-instantiate the canonical type checks for the aggregate type case.

And add all these test cases.

Richard.

>
>	* gnat.dg/discr45.adb: New test.

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

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

> I suggest to re-instantiate the canonical type checks for the aggregate type
> case.

OK, thanks, this fixes all the known ICEs so far.

Tested on x86_64-suse-linux, OK for the mainline?


2015-11-03  Eric Botcazou  <ebotcazou@adacore.com>

	* gimple-expr.c (useless_type_conversion_p): Reinstate type canonical
	check for aggregate types and beef up comment for mode check.


2015-11-03  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/discr45.adb: Only compile the test.

-- 
Eric Botcazou

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

Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 229616)
+++ gimple-expr.c	(working copy)
@@ -84,7 +84,15 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* Changes in machine mode are never useless conversions unless.  */
+  /* For aggregate types, if we know the canonical types, compare them.  This
+     is important for the remapping of variably-modified types.  */
+  if (AGGREGATE_TYPE_P (inner_type)
+      && TYPE_CANONICAL (inner_type)
+      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
+    return true;
+
+  /* Changes in machine mode are never useless conversions because the RTL
+     middle-end expects explicit conversions between modes.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type))
     return false;
 
Index: testsuite/gnat.dg/discr45.adb
===================================================================
--- testsuite/gnat.dg/discr45.adb	(revision 229630)
+++ testsuite/gnat.dg/discr45.adb	(working copy)
@@ -1,4 +1,4 @@
--- { dg-do run }
+-- { dg-do compile }
 -- { dg-options "-O2 -gnatws" }
 
 procedure Discr45 is

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Tue, Nov 3, 2015 at 11:25 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I suggest to re-instantiate the canonical type checks for the aggregate type
>> case.
>
> OK, thanks, this fixes all the known ICEs so far.
>
> Tested on x86_64-suse-linux, OK for the mainline?

Please instead do the change here:

  /* For aggregates compare only the size.  Accesses to fields do have
     a type information by themselves and thus we only care if we can i.e.
     use the types in move operations.  */
  else if (AGGREGATE_TYPE_P (inner_type)
           && TREE_CODE (inner_type) == TREE_CODE (outer_type))
    return (TYPE_MODE (outer_type) != BLKmode
            || operand_equal_p (TYPE_SIZE (inner_type),
                                TYPE_SIZE (outer_type), 0));

to

      return TYPE_CANONICAL (inner_type)
        && TYPE_CANONICAL (outer_type) == TYPE_CANONICAL (inner_type)

Ok with that change.

Richard.

>
> 2015-11-03  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gimple-expr.c (useless_type_conversion_p): Reinstate type canonical
>         check for aggregate types and beef up comment for mode check.
>
>
> 2015-11-03  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/discr45.adb: Only compile the test.
>
> --
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Andreas Schwab]

Eric Botcazou <ebotcazou@adacore.com> writes:

> 	* gnat.dg/discr45.adb: New test.

This fails on ia64.

raised CONSTRAINT_ERROR : discr45.adb:19 range check failed

#0  <__gnat_rcheck_CE_Range_Check> (file=(system.address) 0x4000000000022e98, 
    line=19) at a-except.adb:1286
#1  0x4000000000010470 in discr45.proc (signal=true)
    at /usr/local/gcc/gcc-20151102/gcc/testsuite/gnat.dg/discr45.adb:19
#2  0x40000000000104a0 in discr45 ()
    at /usr/local/gcc/gcc-20151102/gcc/testsuite/gnat.dg/discr45.adb:43

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> This fails on ia64.

gnat.dg/discr44.adb and gnat.dg/discr45.adb are supposed to fail everywhere 
(and there are also a few ACATS failures everywhere).  Sorry for the awkward 
situation but they are testcases exposing the recent type system breakage.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> > This fails on ia64.
> 
> gnat.dg/discr44.adb and gnat.dg/discr45.adb are supposed to fail everywhere 
> (and there are also a few ACATS failures everywhere).  Sorry for the awkward 
> situation but they are testcases exposing the recent type system breakage.

Are these supposed to be fixed by Richard's change to not use useless_type_conversion
for VCE, or is it another issue?

Honza

> 
> -- 
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> Are these supposed to be fixed by Richard's change to not use
> useless_type_conversion for VCE, or is it another issue?

Richard's change not to use useless_type_conversion for VCE was causing 
additional GIMPLE verification failures so I didn't pursue; I can try again,
but all the known regressions are now fixed thanks to Richard's latest change 
to useless_type_conversion_p itself.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> > Are these supposed to be fixed by Richard's change to not use
> > useless_type_conversion for VCE, or is it another issue?
> 
> Richard's change not to use useless_type_conversion for VCE was causing 
> additional GIMPLE verification failures so I didn't pursue; I can try again,
> but all the known regressions are now fixed thanks to Richard's latest change 
> to useless_type_conversion_p itself.

I see, you re-instantiated the TYPE_CANONICAL check for aggregates instead.  I
guess it is most practical way to go right now even though it would be really nice
to separate this from TBAA machinery.
At the moment LTO doesn't do globbing where calling conventions should care.
One such case is the globing of array containing char and char which is required
by Fortran standard, but that IMO is a defect in standard - if types are passed
differently by target ABI one can't expect them to be fuly interoperable as Fortran
would like.

Thank you very much for looking into this!
Honza
> 
> -- 
> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Wed, Nov 4, 2015 at 5:50 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Are these supposed to be fixed by Richard's change to not use
>> > useless_type_conversion for VCE, or is it another issue?
>>
>> Richard's change not to use useless_type_conversion for VCE was causing
>> additional GIMPLE verification failures so I didn't pursue; I can try again,
>> but all the known regressions are now fixed thanks to Richard's latest change
>> to useless_type_conversion_p itself.
>
> I see, you re-instantiated the TYPE_CANONICAL check for aggregates instead.  I
> guess it is most practical way to go right now even though it would be really nice
> to separate this from TBAA machinery.
> At the moment LTO doesn't do globbing where calling conventions should care.
> One such case is the globing of array containing char and char which is required
> by Fortran standard, but that IMO is a defect in standard - if types are passed
> differently by target ABI one can't expect them to be fuly interoperable as Fortran
> would like.

Note that I can't see how non-register type defs/uses will ever
"change" their type
during optimization so I think using TYPE_CANONICAL for the aggregate type case
is fine.

Richard.

> Thank you very much for looking into this!
> Honza
>>
>> --
>> Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

Hi,
here is updated patch that applies changes suggested by Richard. I apologize
for the delay - the testing failed several times on gcc10.fsffrance.org for me
for out-of-memory errors (which are unrelated) and I was on the travel.

Bootstrapped/regtested x86_64-linux, OK?
 
 	* tree.c (verify_type): Verify that TYPE_MODE match
 	between TYPE_CANONICAL and type.
 	* expr.c (store_expr_with_bounds): Revert my previous change.
 	* expmed.c (store_bit_field_1): Revert prevoius change.
 	* gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE
 	to match for all types.

Index: tree.c
===================================================================
--- tree.c	(revision 228933)
+++ tree.c	(working copy)
@@ -13344,6 +13344,14 @@ verify_type (const_tree t)
       error_found = true;
     }
 
+  if (COMPLETE_TYPE_P (t) && TYPE_CANONICAL (t)
+      && TYPE_MODE (t) != TYPE_MODE (TYPE_CANONICAL (t)))
+    {
+      error ("TYPE_MODE of TYPE_CANONICAL is not compatible");
+      debug_tree (ct);
+      error_found = true;
+    }
+
 
   /* Check various uses of TYPE_MINVAL.  */
   if (RECORD_OR_UNION_TYPE_P (t))
Index: expr.c
===================================================================
--- expr.c	(revision 228933)
+++ expr.c	(working copy)
@@ -5425,14 +5425,6 @@ store_expr_with_bounds (tree exp, rtx ta
     temp = convert_modes (GET_MODE (target), TYPE_MODE (TREE_TYPE (exp)),
 			  temp, TYPE_UNSIGNED (TREE_TYPE (exp)));
 
-  /* We allow move between structures of same size but different mode.
-     If source is in memory and the mode differs, simply change the memory.  */
-  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
-    {
-      gcc_assert (MEM_P (temp));
-      temp = adjust_address_nv (temp, GET_MODE (target), 0);
-    }
-
   /* If value was not generated in the target, store it there.
      Convert the value to TARGET's type first if necessary and emit the
      pending incrementations that have been queued when expanding EXP.
Index: expmed.c
===================================================================
--- expmed.c	(revision 228933)
+++ expmed.c	(working copy)
@@ -757,14 +757,6 @@ store_bit_field_1 (rtx str_rtx, unsigned
       }
   }
 
-  /* We allow move between structures of same size but different mode.
-     If source is in memory and the mode differs, simply change the memory.  */
-  if (GET_MODE (value) == BLKmode && GET_MODE (op0) != BLKmode)
-    {
-      gcc_assert (MEM_P (value));
-      value = adjust_address_nv (value, GET_MODE (op0), 0);
-    }
-
   /* Storing an lsb-aligned field in a register
      can be done with a movstrict instruction.  */
 
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 228933)
+++ gimple-expr.c	(working copy)
@@ -87,10 +87,8 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* Changes in machine mode are never useless conversions unless we
-     deal with aggregate types in which case we defer to later checks.  */
-  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
-      && !AGGREGATE_TYPE_P (inner_type))
+  /* Changes in machine mode are never useless conversions unless.  */
+  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type))
     return false;
 
   /* If both the inner and outer types are integral types, then the
@@ -270,10 +268,9 @@ useless_type_conversion_p (tree outer_ty
      use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
 	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return (!TYPE_SIZE (outer_type)
-	    || (TYPE_SIZE (inner_type)
-		&& operand_equal_p (TYPE_SIZE (inner_type),
-				    TYPE_SIZE (outer_type), 0)));
+    return (TYPE_MODE (outer_type) != BLKmode
+	    || operand_equal_p (TYPE_SIZE (inner_type),
+			        TYPE_SIZE (outer_type), 0));
 
   else if (TREE_CODE (inner_type) == OFFSET_TYPE
 	   && TREE_CODE (outer_type) == OFFSET_TYPE)

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Wed, Oct 21, 2015 at 6:05 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> here is updated patch that applies changes suggested by Richard. I apologize
> for the delay - the testing failed several times on gcc10.fsffrance.org for me
> for out-of-memory errors (which are unrelated) and I was on the travel.
>
> Bootstrapped/regtested x86_64-linux, OK?

Ok.

Thanks,
Richard.

>         * tree.c (verify_type): Verify that TYPE_MODE match
>         between TYPE_CANONICAL and type.
>         * expr.c (store_expr_with_bounds): Revert my previous change.
>         * expmed.c (store_bit_field_1): Revert prevoius change.
>         * gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE
>         to match for all types.
>
> Index: tree.c
> ===================================================================
> --- tree.c      (revision 228933)
> +++ tree.c      (working copy)
> @@ -13344,6 +13344,14 @@ verify_type (const_tree t)
>        error_found = true;
>      }
>
> +  if (COMPLETE_TYPE_P (t) && TYPE_CANONICAL (t)
> +      && TYPE_MODE (t) != TYPE_MODE (TYPE_CANONICAL (t)))
> +    {
> +      error ("TYPE_MODE of TYPE_CANONICAL is not compatible");
> +      debug_tree (ct);
> +      error_found = true;
> +    }
> +
>
>    /* Check various uses of TYPE_MINVAL.  */
>    if (RECORD_OR_UNION_TYPE_P (t))
> Index: expr.c
> ===================================================================
> --- expr.c      (revision 228933)
> +++ expr.c      (working copy)
> @@ -5425,14 +5425,6 @@ store_expr_with_bounds (tree exp, rtx ta
>      temp = convert_modes (GET_MODE (target), TYPE_MODE (TREE_TYPE (exp)),
>                           temp, TYPE_UNSIGNED (TREE_TYPE (exp)));
>
> -  /* We allow move between structures of same size but different mode.
> -     If source is in memory and the mode differs, simply change the memory.  */
> -  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
> -    {
> -      gcc_assert (MEM_P (temp));
> -      temp = adjust_address_nv (temp, GET_MODE (target), 0);
> -    }
> -
>    /* If value was not generated in the target, store it there.
>       Convert the value to TARGET's type first if necessary and emit the
>       pending incrementations that have been queued when expanding EXP.
> Index: expmed.c
> ===================================================================
> --- expmed.c    (revision 228933)
> +++ expmed.c    (working copy)
> @@ -757,14 +757,6 @@ store_bit_field_1 (rtx str_rtx, unsigned
>        }
>    }
>
> -  /* We allow move between structures of same size but different mode.
> -     If source is in memory and the mode differs, simply change the memory.  */
> -  if (GET_MODE (value) == BLKmode && GET_MODE (op0) != BLKmode)
> -    {
> -      gcc_assert (MEM_P (value));
> -      value = adjust_address_nv (value, GET_MODE (op0), 0);
> -    }
> -
>    /* Storing an lsb-aligned field in a register
>       can be done with a movstrict instruction.  */
>
> Index: gimple-expr.c
> ===================================================================
> --- gimple-expr.c       (revision 228933)
> +++ gimple-expr.c       (working copy)
> @@ -87,10 +87,8 @@ useless_type_conversion_p (tree outer_ty
>    if (inner_type == outer_type)
>      return true;
>
> -  /* Changes in machine mode are never useless conversions unless we
> -     deal with aggregate types in which case we defer to later checks.  */
> -  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
> -      && !AGGREGATE_TYPE_P (inner_type))
> +  /* Changes in machine mode are never useless conversions unless.  */
> +  if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type))
>      return false;
>
>    /* If both the inner and outer types are integral types, then the
> @@ -270,10 +268,9 @@ useless_type_conversion_p (tree outer_ty
>       use the types in move operations.  */
>    else if (AGGREGATE_TYPE_P (inner_type)
>            && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> -    return (!TYPE_SIZE (outer_type)
> -           || (TYPE_SIZE (inner_type)
> -               && operand_equal_p (TYPE_SIZE (inner_type),
> -                                   TYPE_SIZE (outer_type), 0)));
> +    return (TYPE_MODE (outer_type) != BLKmode
> +           || operand_equal_p (TYPE_SIZE (inner_type),
> +                               TYPE_SIZE (outer_type), 0));
>
>    else if (TREE_CODE (inner_type) == OFFSET_TYPE
>            && TREE_CODE (outer_type) == OFFSET_TYPE)

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Eric Botcazou]

> here is updated patch that applies changes suggested by Richard. I apologize
> for the delay - the testing failed several times on gcc10.fsffrance.org for
> me for out-of-memory errors (which are unrelated) and I was on the travel.
> 
> Bootstrapped/regtested x86_64-linux, OK?
> 
>  	* tree.c (verify_type): Verify that TYPE_MODE match
>  	between TYPE_CANONICAL and type.
>  	* expr.c (store_expr_with_bounds): Revert my previous change.
>  	* expmed.c (store_bit_field_1): Revert prevoius change.
>  	* gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE
>  	to match for all types.

Please add the PR middle-end/67966 reference to the ChangeLog.

-- 
Eric Botcazou

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

Hello,
this is a variant of patch I tested.  After looking into the issue more, I think we don't really need
to check types to be compatible (or we want to check it in other references, too).  It seems to me
that we should be able to drop 
              /* Verify that access happens in similar types.  */               
              if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))     
                return 0;                                                       
from MEM_REF, too.

I had bit hard time creating a testcase running into an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68062
With C FE I only know how to produce VCEs by vector conversions.  Will play
with it more tomorrow.  The code patch hits about 1k times during bootstrap.

Bootstrapped/regtested ppc64-linux, OK?

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 228933)
+++ fold-const.c	(working copy)
@@ -2960,6 +2962,7 @@ operand_equal_p (const_tree arg0, const_
 
 	case REALPART_EXPR:
 	case IMAGPART_EXPR:
+	case VIEW_CONVERT_EXPR:
 	  return OP_SAME (0);
 
 	case TARGET_MEM_REF:

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener]

On Fri, Oct 23, 2015 at 7:19 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hello,
> this is a variant of patch I tested.  After looking into the issue more, I think we don't really need
> to check types to be compatible (or we want to check it in other references, too).  It seems to me
> that we should be able to drop
>               /* Verify that access happens in similar types.  */
>               if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
>                 return 0;
> from MEM_REF, too.

Yeah.

> I had bit hard time creating a testcase running into an ICE
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68062
> With C FE I only know how to produce VCEs by vector conversions.  Will play
> with it more tomorrow.  The code patch hits about 1k times during bootstrap.
>
> Bootstrapped/regtested ppc64-linux, OK?

Ok.

Thanks,
Richard.

> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 228933)
> +++ fold-const.c        (working copy)
> @@ -2960,6 +2962,7 @@ operand_equal_p (const_tree arg0, const_
>
>         case REALPART_EXPR:
>         case IMAGPART_EXPR:
> +       case VIEW_CONVERT_EXPR:
>           return OP_SAME (0);
>
>         case TARGET_MEM_REF:

Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Jan Hubicka]

> >         * fold-const.c (operand_equal_p): Handle VIEW_CONVERT_EXPR.
> > Index: fold-const.c
> > ===================================================================
> > --- fold-const.c        (revision 228735)
> > +++ fold-const.c        (working copy)
> > @@ -2962,6 +2968,12 @@ operand_equal_p (const_tree arg0, const_
> >         case IMAGPART_EXPR:
> >           return OP_SAME (0);
> >
> > +       case VIEW_CONVERT_EXPR:
> > +         if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
> > +             && !types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
> 
> Note that mixing the GIMPLE types_compatible_p into operand_equal_p which
> is supposed to handle GENERIC as well looks dangerous.  You short-cutted
> the type checks for OEP_ADDRESS_OF earlier so why do you need to
> preserve them here?
> 
> The test looks bogus anyway, but maybe it's just too early in the morning ;)
> Shouldn't it be && types_compatible_p (...) (instead of && !types_com...)?
> Otherwise it looks really weird.

I think it is as intended: If we do !OEP_ADDRESS_OF we want to know that types
are compatible. 

I am not quite sure this is needed - I wanted to mention that in an email, but
it was too late in night for me :)

In NOP_EXPR we check:

      /* Two conversions are equal only if signedness and modes match.  */
      switch (TREE_CODE (arg0))
        {
        CASE_CONVERT:
        case FIX_TRUNC_EXPR:
          if (TYPE_UNSIGNED (TREE_TYPE (arg0))
              != TYPE_UNSIGNED (TREE_TYPE (arg1)))
            return 0;
          break;
        default:
          break;
        }

And earlier we do:

      /* If both types don't have the same signedness, then we can't consider
         them equal.  We must check this before the STRIP_NOPS calls
         because they may change the signedness of the arguments.  As pointers
         strictly don't have a signedness, require either two pointers or
         two non-pointers as well.  */
      if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE (arg1))
          || POINTER_TYPE_P (TREE_TYPE (arg0))
                             != POINTER_TYPE_P (TREE_TYPE (arg1)))
        return 0;

So except for NOP_EXPR being stripped early this is bit redundant.

I wondered what happens if I match two VCEs of very different type. Say
vector of FLOATs and vector of INTEGERs and then convert them to same type that
would result in differnt value. But this is not terribly special for VCE
and probably would affect other expressions, too.

I noticed now that this probably this can't happen because we also do:

  /* This is needed for conversions and for COMPONENT_REF.
     Might as well play it safe and always test this.  */
  if (TREE_CODE (TREE_TYPE (arg0)) == ERROR_MARK
      || TREE_CODE (TREE_TYPE (arg1)) == ERROR_MARK
      || TYPE_MODE (TREE_TYPE (arg0)) != TYPE_MODE (TREE_TYPE (arg1)))
    return 0;

Which sounds somewhat conservative. At least should be skipped for OEP_ADDRESS_OF.
You earlier mentioned BLKmode vectors. What would happen in this case?

If I remember correctly. the check was not needed to pass testing, I am respawning testing
without the type check.

Honza
> 
> Richard.
> 
> > +           return false;
> > +         return OP_SAME (0);
> > +
> >         case TARGET_MEM_REF:
> >         case MEM_REF:
> >           if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)))