Bug 13954 - [tree-ssa] SRA does not work for classes that use inheritance with an empty base
Summary: [tree-ssa] SRA does not work for classes that use inheritance with an empty base
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: tree-ssa
: P2 enhancement
Target Milestone: 4.7.0
Assignee: Richard Biener
URL:
Keywords: missed-optimization
Depends on:
Blocks: 22501
  Show dependency treegraph
 
Reported: 2004-01-31 21:15 UTC by Dan Nicolaescu
Modified: 2011-03-15 13:41 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-09-26 15:30:16


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Nicolaescu 2004-01-31 21:15:34 UTC
SRA does not work for classes that use inheritance.
This is a known issue, filed so that it can be tracked. 

/* { dg-do compile } */ 
/* { dg-options "-O1 -fdump-tree-optimized" } */

void link_error (void);

class base
{
};

class teststruct: public base
{
public:
  double d;
  char f1;
};

void
copystruct1 (teststruct param)
{
  teststruct local;
  param.f1 = 0;
  local = param;
  if (local.f1 != 0)
    link_error ();
}

/* There should be no reference to link_error. */
/* { dg-final { scan-tree-dump-times "link_error" 0 "optimized"} } */
Comment 1 Andrew Pinski 2004-01-31 21:23:55 UTC
Confirmed, the problem here is more complicated:
Cannot scalarize variable param because it must live in memory
Cannot scalarize variable local because it must live in memory
Cannot scalarize variable MT.3 because it must live in memory

The problem here is that:
void copystruct1(teststruct) (param)
{
  struct 
  {
    double d;
    char f1;
  } * local.0;
  struct 
  {
    double d;
    char f1;
  } * param.1;
  char T.2;

  {
    struct teststruct local;

    param.f1 = 0;
    local.0 = (struct 
    {
      double d;
      char f1;
    } *)&local;
    param.1 = (struct 
    {
      double d;
      char f1;
    } *)&param;
    *local.0 = *param.1;
    {
      T.2 = local.f1;
      if (T.2 != 0)
        {
          {
            link_error ();
          }
        }
      else
        {
          
        }
    }
  }
}

Which means it does not using the right structs assigning or something werid is going on.
Comment 2 Dan Nicolaescu 2004-02-01 00:04:20 UTC
This:
> Cannot scalarize variable param because it must live in memory

happens because for "param"  is_gimple_non_addressable returns true 
because "param" has the TREE_ADDRESSABLE bit set. 
Comment 3 Diego Novillo 2004-02-01 07:39:43 UTC
Mine.
Comment 4 Richard Henderson 2004-02-03 03:25:32 UTC
Happens because the C++ front end is amazingly stupid about how it
emits access to members that are not located in virtual bases.

Fixing this requires rearranging how offsetof is handled.
Comment 5 Andrew Pinski 2004-02-23 16:29:55 UTC
Mine as I am fixing offsetof and some of the C++ front-end also.
Comment 6 Andrew Pinski 2004-05-14 21:55:44 UTC
This is a C++ front-end issue.
Comment 7 Richard Henderson 2004-06-29 21:27:40 UTC
Testing a patch.
Comment 8 Richard Henderson 2004-06-29 21:38:45 UTC
Well, no I'm not.  This is a different problem than I thought.

This is the case of the C++ front end wanting to perform a block copy between
two structures, but *without* copying the trailing padding of the structure.
It decides that the best way to do this is to cast the two structures to an
internal type that doesn't include the padding.

Presumably this is to handle cases in which someone else inherits from 
"teststruct", and reuses the tail padding.  As inheritence is allowed to do.

I almost think that just using __builtin_memcpy would be a better representation,
but that wouldn't have any effect on the scalarizability of this test case.

At least yet.
Comment 9 Giovanni Bajo 2004-06-30 02:55:56 UTC
Exactly. teststruct is a non-POD (because it derives from base), so you can't 
touch its padding because it could contain data.
Comment 10 Richard Henderson 2004-06-30 20:07:30 UTC
For the record, the cast-vs-memcpy thing has been fixed.
http://gcc.gnu.org/ml/gcc-patches/2004-06/msg02647.html
Comment 11 Richard Henderson 2004-12-16 07:32:17 UTC
Not working on it.
Comment 12 Richard Biener 2006-03-06 14:41:59 UTC
In principle this blocks optimization of tramp3d domain operations (if it were not structure-aliasing fixing most of the problems).
Comment 13 Richard Biener 2006-10-24 12:34:46 UTC
We now get

<bb 2>:
  #   param_2 = V_MAY_DEF <param_1>;
  param.f1 = 0;
  #   param_6 = V_MAY_DEF <param_2>;
  #   SFT.0_7 = V_MAY_DEF <SFT.0_3>;
  #   NONLOCAL.6_8 = V_MAY_DEF <NONLOCAL.6_5>;
  #   NONLOCAL.12_13 = V_MAY_DEF <NONLOCAL.12_12>;
  #   NONLOCAL.18_16 = V_MAY_DEF <NONLOCAL.18_15>;
  #   NONLOCAL.24_19 = V_MAY_DEF <NONLOCAL.24_18>;
  #   NONLOCAL.30_22 = V_MAY_DEF <NONLOCAL.30_21>;
  #   NONLOCAL.36_25 = V_MAY_DEF <NONLOCAL.36_24>;
  __builtin_memcpy (&local, &param, 9);
  #   VUSE <SFT.0_7>;
  D.2668_4 = local.f1;
  if (D.2668_4 != 0) goto <L0>; else goto <L1>;

<L0>:;
  #   param_9 = V_MAY_DEF <param_6>;
  #   SFT.0_10 = V_MAY_DEF <SFT.0_7>;
  #   NONLOCAL.6_11 = V_MAY_DEF <NONLOCAL.6_8>;
  #   NONLOCAL.12_14 = V_MAY_DEF <NONLOCAL.12_13>;
  #   NONLOCAL.18_17 = V_MAY_DEF <NONLOCAL.18_16>;
  #   NONLOCAL.24_20 = V_MAY_DEF <NONLOCAL.24_19>;
  #   NONLOCAL.30_23 = V_MAY_DEF <NONLOCAL.30_22>;
  #   NONLOCAL.36_26 = V_MAY_DEF <NONLOCAL.36_25>;
  link_error ();

<L1>:;
  return;
Comment 14 Daniel Berlin 2006-10-24 18:31:03 UTC
(In reply to comment #13)
> We now get
> 
> <bb 2>:
>   #   param_2 = V_MAY_DEF <param_1>;
>   param.f1 = 0;
>   #   param_6 = V_MAY_DEF <param_2>;
>   #   SFT.0_7 = V_MAY_DEF <SFT.0_3>;
>   #   NONLOCAL.6_8 = V_MAY_DEF <NONLOCAL.6_5>;
>   #   NONLOCAL.12_13 = V_MAY_DEF <NONLOCAL.12_12>;
>   #   NONLOCAL.18_16 = V_MAY_DEF <NONLOCAL.18_15>;
>   #   NONLOCAL.24_19 = V_MAY_DEF <NONLOCAL.24_18>;
>   #   NONLOCAL.30_22 = V_MAY_DEF <NONLOCAL.30_21>;
>   #   NONLOCAL.36_25 = V_MAY_DEF <NONLOCAL.36_24>;
>   __builtin_memcpy (&local, &param, 9);
>   #   VUSE <SFT.0_7>;
>   D.2668_4 = local.f1;
>   if (D.2668_4 != 0) goto <L0>; else goto <L1>;
> 
> <L0>:;
>   #   param_9 = V_MAY_DEF <param_6>;
>   #   SFT.0_10 = V_MAY_DEF <SFT.0_7>;
>   #   NONLOCAL.6_11 = V_MAY_DEF <NONLOCAL.6_8>;
>   #   NONLOCAL.12_14 = V_MAY_DEF <NONLOCAL.12_13>;
>   #   NONLOCAL.18_17 = V_MAY_DEF <NONLOCAL.18_16>;
>   #   NONLOCAL.24_20 = V_MAY_DEF <NONLOCAL.24_19>;
>   #   NONLOCAL.30_23 = V_MAY_DEF <NONLOCAL.30_22>;
>   #   NONLOCAL.36_26 = V_MAY_DEF <NONLOCAL.36_25>;
>   link_error ();
> 
> <L1>:;
>   return;
> 

Uh, there should only ever be one non-local var per function, and referenced vars should be reset at the end of each function so why do you have multiple NONLOCAL's here?
Comment 15 Richard Biener 2009-09-26 15:30:15 UTC
On trunk we get

<bb 2>:
  # .MEM_3 = VDEF <.MEM_2(D)>
  param.f1 = 0;
  # .MEM_4 = VDEF <.MEM_3>
  memcpy (&local, &param, 9);
  # VUSE <.MEM_4>
  D.1743_1 = local.f1;
  if (D.1743_1 != 0)
    goto <bb 3>;
  else
    goto <bb 4>;

<bb 3>:
  # .MEM_5 = VDEF <.MEM_4>
  link_error ();

<bb 4>:
  return;

which shows this is an issue of value-numbering not looking through
memcpy.  SRA obviously does not work here because both vars have their
address taken.  The issue in the summary should have been fixed by the
SRA rewrite.

I'm looking at the VN issue.
Comment 16 Richard Biener 2010-08-11 12:33:42 UTC
We can also expand

  __builtin_memcpy (&local, &param, 9);

to multiple copies based on src/dest alignment and size (similar to
store_by_pieces)
Comment 17 Richard Biener 2011-03-08 15:50:59 UTC
I have a patch, queued for 4.7.
Comment 18 Richard Biener 2011-03-15 13:37:42 UTC
Author: rguenth
Date: Tue Mar 15 13:37:23 2011
New Revision: 170994

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170994
Log:
2011-03-15  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/13954
	* tree-ssa-sccvn.c (vn_reference_lookup_3): Look through memcpy
	and friends.

	* g++.dg/tree-ssa/pr13954.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr13954.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-sccvn.c
Comment 19 Richard Biener 2011-03-15 13:41:43 UTC
Fixed.