[Bug ipa/98594] New: [11 Regression] IPA modref codegen bug

[Bug ipa/98594] New: [11 Regression] IPA modref codegen bug

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

            Bug ID: 98594
           Summary: [11 Regression] IPA modref codegen bug
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: law at redhat dot com
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---
            Target: s390x

Created attachment 49917
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49917&action=edit
Hacked up/reduced testcase

The "glm" package on s390x fails to build on s390x due to what appears to be a
bug in the IPA modref changes.  Specifically:

commit 67a5c215940f4b21bac1aa489ce1f2fb3d52a53a
Author: Jan Hubicka <jh@suse.cz>
Date:   Sat Sep 26 00:01:57 2020 +0200

    Fix gimple_clobber handling in ipa-modref

    2020-09-25  Jan Hubicka  <hubicka@ucw.cz>

            * ipa-modref.c (analyze_stmt): Fix return value for
gimple_clobber.

But I suspect this is just exposing a bug elsewhere in the modref support.

Before early inlining we have:

int bitCount::test ()
{
  int _1;
  int _2;
  int _3;
  bool _4;
  int _7;
  int _8;

  <bb 2> :
  _1 = 1;
  _2 = 1;
  _7 = bitCount::bitCount_bitfield<int> (_2);
  _3 = _7;
  _4 = _1 == _3;
  _8 = (int) _4;
  return _8;

}

Nothing really interesting there.  THe called function looks like:

;; Function bitCount::bitCount_bitfield<int>
(_ZN8bitCountL17bitCount_bitfieldIiEEiT_, funcdef_no=13, decl_uid=3087,
cgraph_uid=2, symbol_order=4)


Analyzing function body size: int bitCount::bitCount_bitfield(genType)
[with genType = int]/4

int bitCount::bitCount_bitfield<int> (int x)
{
  struct vec D.3185;
  struct vec D.3199;
  int _5;

  <bb 2> :
  glm::vec<1, int, glm::packed_highp>::vec (&D.3185, x_2(D));
  D.3199 = bitCount::bitCount_bitfield<1, int, glm::packed_highp>
(&D.3185); [return slot optimization]
  _5 = D.3199.D.3097.x;
  D.3199 ={v} {CLOBBER};
  D.3185 ={v} {CLOBBER};
  return _5;

}


The most important thing to note here is that the local variable D.3185
and the parameter X get passed to the glm::vec constructor.  D.3185 then
gets passed to an overload of bitCount::bitCount_bitfield.

The constructor looks like:

;; Function glm::vec<1, int, glm::packed_highp>::vec
(_ZN3glm3vecILi1EiLNS_9qualifierE0EEC1Ei, funcdef_no=16, decl_uid=3159,
cgraph_uid=5, symbol_order=7)


void glm::vec<1, int, glm::packed_highp>::vec (struct vec * const this,
int scalar)
{
  <bb 2> :
  *this_2(D) ={v} {CLOBBER};
  this_2(D)->D.3097.x = scalar_4(D);
  return;

}

So we take the 2nd argument and stuff it into a field within the first
argument.  This all looks good.  Now we look at the
bitCount::bitCount_bitfield overload (edited for brevity):

;; Function bitCount::bitCount_bitfield<1, int, glm::packed_highp>
(_ZN8bitCountL17bitCount_bitfieldILi1EiLN3glm9qualifierE0EEENS1_3vecIXT_EiXT1_EEERKNS3_IXT_ET0_XT1_EEE,
funcdef_no=17, decl_uid=3197, cgraph_uid=6, symbol_order=8)

struct vec bitCount::bitCount_bitfield<1, int, glm::packed_highp> (const
struct vec & v)
{
  struct vec x;

  <bb 2> :
  x = MEM[(const struct vec &)v_2(D)];
  x = bitCount::compute_bitfieldBitCountStep<true>::call<1, unsigned
int, glm::packed_highp> (&x, 1431655765, 1);
  x = bitCount::compute_bitfieldBitCountStep<true>::call<1, unsigned
int, glm::packed_highp> (&x, 858993459, 2);
  x = bitCount::compute_bitfieldBitCountStep<true>::call<1, unsigned
int, glm::packed_highp> (&x, 252645135, 4);
  glm::vec<1, int, glm::packed_highp>::vec<unsigned int,
glm::packed_highp> (&<retval>, &x);
  x ={v} {CLOBBER};
  return <retval>;

}

So note the first real statement.  That's going to read the data that
was put into the vec node by the constructor. All good.

Now in the .einline dump things have gone to hell.
;; Function bitCount::test (_ZN8bitCount4testEv, funcdef_no=12,
decl_uid=3085, cgraph_uid=1, symbol_order=3)

Iterations: 1

Symbols to be put in SSA form
{ D.3471 D.3522 }
Incremental SSA update started at block: 0
Number of blocks in CFG: 5
Number of blocks to update: 4 ( 80%)


Merging blocks 2 and 4
Merging blocks 2 and 3
int bitCount::test ()
{
  struct vec D.3524;
  struct vec D.3523;
  int D.3522;
  int _1;
  int _2;
  int _3;
  bool _4;
  int _6;
  int _7;
  int _8;

  <bb 2> :
  _1 = 1;
  _2 = 1;
  D.3524 = bitCount::bitCount_bitfield<1, int, glm::packed_highp>
(&D.3523); [return slot optimization]
  _6 = D.3524.D.3097.x;
  D.3524 ={v} {CLOBBER};
  D.3523 ={v} {CLOBBER};
  _12 = _6;
  _7 = _12;
  _3 = _7;
  _4 = _1 == _3;
  _8 = (int) _4;
  return _8;

}

Note we pass D.3523 to the bitCount_bitfield overload, but we've lost
the statement that initialized its data field. Naturally the overload
still loads that (now uninitialized) field and we get garbage data and
the testsuite fails miserably.  FUrthermore, we get an uninitalized
warning (perhaps your subsequent uninit changes were papering over this
problem to some degree).

You should be able to see this with an s390x cross compiler with -O2.

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
                 CC|                            |hubicka at gcc dot gnu.org
   Target Milestone|---                         |11.0

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
                 CC|                            |rguenth at gcc dot gnu.org
   Last reconfirmed|                            |2021-01-08
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
We have a dup of modref mishandling RSO, but this testcase is nicer I guess. 
Honza - please have a look.

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |hubicka at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #2 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Mine.  Note that the initialized warning was not really related to this
problem, just to the assumption that parameters declared "const" are read by
the callee which was proved false by modref and triggered false positive in the
warning.

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

--- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The initialization is removed by dse1 pass.  We get:
ipa-modref: call stmt D.3199 = bitCount::bitCount_bitfield<1, int,
glm::packed_highp> (&D.3185); [return slot optimization]
ipa-modref: call to glm::vec<L, int, Q> bitCount::bitCount_bitfield(const
glm::vec<L, T, Q>&) [with int L = 1; T = int; glm::qualifier Q =
glm::packed_highp]/8 does not use ref: D.3185.D.3097.x alias sets: 3->1
  Deleted dead store: D.3185.D.3097.x = x_2(D);                                 

ipa-modref: call stmt D.3199 = bitCount::bitCount_bitfield<1, int,
glm::packed_highp> (&D.3185); [return slot optimization]
ipa-modref: call to glm::vec<L, int, Q> bitCount::bitCount_bitfield(const
glm::vec<L, T, Q>&) [with int L = 1; T = int; glm::qualifier Q =
glm::packed_highp]/8 does not use ref: D.3185 alias sets: 3->3
  Deleted dead store: D.3185 ={v} {CLOBBER};                                    

Now the modref summary for function is
  loads:                                                                        
    Limits: 32 bases, 16 refs                                                   
      Base 0: alias set 5                                                       
        Ref 0: alias set 5                                                      
          access: Parm 0 param offset:0 offset:0 size:32 max_size:32            

alias set 5 correspond to const struct vec but diferent instantiation than
alias set 3 used in the store.
There is reinterpret cast:

  glm::vec<L, typename glm::detail::make_unsigned<T>::type,
Q>x(*reinterpret_cast<glm::vec<L, typename glm::detail::make_unsigned<T>::type,
Q> const *>(&v));

turning it to

  glm::vec<L, typename glm::detail::make_unsigned<T>::type, Q> x(*(&v));

makes the aliasing difference go away.  So it seems to me that the testcase
simply includes TBAA violation?

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

--- Comment #4 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 27 Jan 2021, hubicka at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594
> 
> --- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
> The initialization is removed by dse1 pass.  We get:
> ipa-modref: call stmt D.3199 = bitCount::bitCount_bitfield<1, int,
> glm::packed_highp> (&D.3185); [return slot optimization]
> ipa-modref: call to glm::vec<L, int, Q> bitCount::bitCount_bitfield(const
> glm::vec<L, T, Q>&) [with int L = 1; T = int; glm::qualifier Q =
> glm::packed_highp]/8 does not use ref: D.3185.D.3097.x alias sets: 3->1
>   Deleted dead store: D.3185.D.3097.x = x_2(D);                                 
> 
> ipa-modref: call stmt D.3199 = bitCount::bitCount_bitfield<1, int,
> glm::packed_highp> (&D.3185); [return slot optimization]
> ipa-modref: call to glm::vec<L, int, Q> bitCount::bitCount_bitfield(const
> glm::vec<L, T, Q>&) [with int L = 1; T = int; glm::qualifier Q =
> glm::packed_highp]/8 does not use ref: D.3185 alias sets: 3->3
>   Deleted dead store: D.3185 ={v} {CLOBBER};                                    
> 
> Now the modref summary for function is
>   loads:                                                                        
>     Limits: 32 bases, 16 refs                                                   
>       Base 0: alias set 5                                                       
>         Ref 0: alias set 5                                                      
>           access: Parm 0 param offset:0 offset:0 size:32 max_size:32            
> 
> alias set 5 correspond to const struct vec but diferent instantiation than
> alias set 3 used in the store.
> There is reinterpret cast:
> 
>   glm::vec<L, typename glm::detail::make_unsigned<T>::type, Q>
      x(*reinterpret_cast<
    glm::vec<L, typename glm::detail::make_unsigned<T>::type, Q> const *>(&v));
> 
> turning it to
> 
>   glm::vec<L, typename glm::detail::make_unsigned<T>::type, Q> x(*(&v));
> 
> makes the aliasing difference go away.  So it seems to me that the testcase
> simply includes TBAA violation?

Not sure but if my visuals do not cheat me then the difference is only
const qualification so it should not matter for TBAA?  Of course the
question is what type 'v' has since this maybe invokes a different CTOR?

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Hmm, I don't think struct X { int i; } and struct X { unsigned int i; } have
the same alias-set (int and unsigned int do).  So a reinterpret-cast
from one to the other variant violates TBAA rules.  That menas that glm::vec
needs a CTOR from the signed/unsigned variant rather than using a hack
like this - and it seems it has one if Honzas fix still lets things compile?
I see

 template<typename T, qualifier Q>
 template<typename U, qualifier P>
 inline constexpr vec<1, T, Q>::vec(vec<1, U, P> const& v)
  : x(static_cast<T>(v.x))
 {}

which probably applies and the used CTOR in the bogus case is the
compiler-generated one.

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[law at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

--- Comment #6 from Jeffrey A. Law <law at redhat dot com> ---
Duh.  I should have seen the reinterpret_cast as a red flag on this one.  And
not surprising -fno-strict-aliasing makes the glm testsuite happy.  Sorry for
the noise.

[Bug ipa/98594] [11 Regression] IPA modref codegen bug

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98594

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |slyfox at gcc dot gnu.org

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
*** Bug 102823 has been marked as a duplicate of this bug. ***