Bug 31081 - [4.3 Regression] Inliner messes up SSA for abnormals
[4.3 Regression] Inliner messes up SSA for abnormals
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: tree-optimization
4.3.0
: P1 blocker
: 4.3.0
Assigned To: Jan Hubicka
: ice-on-valid-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-08 12:32 UTC by Andrew Pinski
Modified: 2008-01-04 11:16 UTC (History)
10 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.2.0
Known to fail: 4.3.0
Last reconfirmed: 2007-10-28 19:09:04


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Pinski 2007-03-08 12:32:25 UTC
Testcase:
static int get_record (void);
void f(void);
int g(void);
static int get_record (void)
{
  int     result;
  try
  {
    result = g();
    f();
  }
  catch (const int &)   { }
  return result;
}
int NAV_get_record ( )
{
  int     result;
  for (;;)
    if (get_record ())
      return 1;
}
-------------
Unable to coalesce ssa_names 9  and 5  which are marked as MUST COALESCE.
result_9(ab) and  result_5(ab)

t.cc: In function 'int NAV_get_record()':
t.cc:5: internal compiler error: SSA corruption
Comment 1 Andrew Pinski 2007-03-08 12:33:00 UTC
Confirmed, this was just a splitting of PR 30604 into this one (with reducing).
Comment 2 Andrew Pinski 2007-03-08 12:37:15 UTC
Note if I manually inline NAV_get_record, I get an extra PHI:
  # result_1(ab) = PHI <result_4(D)(0), result_3(5)>


Where result_5 would be the same as result_1 here.
Comment 3 Janis Johnson 2007-10-22 16:34:43 UTC
A regression hunt on powerpc-linux identified:

    http://gcc.gnu.org/viewcvs?view=rev&rev=120373

    r120373 | hubicka | 2007-01-03 01:12:56 +0000 (Wed, 03 Jan 2007)
Comment 4 Jakub Staszak 2007-10-22 22:34:37 UTC
(In reply to comment #3)
> A regression hunt on powerpc-linux identified:
> 
>     http://gcc.gnu.org/viewcvs?view=rev&rev=120373
> 
>     r120373 | hubicka | 2007-01-03 01:12:56 +0000 (Wed, 03 Jan 2007)
> 

This patch causes error because it changes order of passes; pass_build_ssa is much earlier and we have inlining in ssa. This is correct (I think), and this patch is OK. Problem is (probably) in inliner.
Comment 5 Jan Hubicka 2007-10-28 19:09:04 UTC
Mine, it is another example of EH updating problem.  I will try to add "may" EH edges pre-inlniing to avoid need to call mark_sym_for_renaming in inliner completely.
Comment 6 victork 2007-10-30 06:51:14 UTC
I'm not sure, but it looks like PR33319 is duplicate of this one.
(pr27549.C when compiled with -ftree-vectorize).
Comment 7 Richard Biener 2007-10-31 12:24:30 UTC
Another, similar testcase.  ICEs at -O and above with

Unable to coalesce ssa_names 23 and 15 which are marked as MUST COALESCE.
SR.8_23(ab) and  SR.8_15(ab)
t.ii: In function 'int main(int, char**)':
t.ii:36: internal compiler error: SSA corruption
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.


class String;

class CString
{
public:
    CString();
    ~CString() { operator delete(_rep); }
    operator const char*() const { return _rep; }
private:
    CString(char* cstr);
    char* _rep;
};

class String
{
public:

    String();
    String(const char* str);
    ~String();
    CString getCString() const;
};

int is_absolute_path(const char *path);

inline void getAbsolutePath(
    const char* path,
    const String& filename)
{
    (!is_absolute_path(filename.getCString()) && path);
    return;
}

int foo(int &value);

int main(int argc, char** argv)
{
    int repeatTestCount = 0;
    if (foo(repeatTestCount))
    {
        repeatTestCount = 1;
    }
    for (int numTests = 1; numTests <= repeatTestCount; numTests++)
    {
            getAbsolutePath("blah", "blah");
    }
    return 0;
}
Comment 8 Jakub Staszak 2007-11-16 01:19:21 UTC
i've tried to understand it but i can't. Can anyone explain me..
Why does coalesce_partitions (tree-ssa-coalesce.c) fail after unsuccessful attempt_coalesce? what is the difference if it is abnormal edge, or not?
Comment 9 Andrew Pinski 2007-12-03 02:44:07 UTC
From PR 30604:
 OK, new investigation show that using the smaller testcase firebird2-nav.cc
shows the inliner is misbehaving. 

before inlining  (*041t.profile):

  # BLOCK 2 freq:10000
  # PRED: ENTRY [100.0%]  (fallthru,exec)
  D.2709_4 = request_3(D)->req_pool;
  D.2710_5 = (struct blk *) D.2709_4;
  D.2711_6 = request_3(D)->req_transaction;
  D.2712_10 = VIO_get (tdbb_7(D), rpb_8(D), rsb_9(D), D.2711_6, D.2710_5);
  # SUCC: 4 (ab,eh,exec) 3 [100.0%]  (fallthru,exec)

  # BLOCK 3 freq:10000
  # PRED: 2 [100.0%]  (fallthru,exec)
  result_12(ab) = (BOOLEAN) D.2712_10;
<...>
  BTR_key (tdbb_7(D), D.2719_20, D.2718_19, D.2717_18, &value, 0B);
  goto <bb 5>;
  # SUCC: 4 (ab,eh,exec) 5 [100.0%]  (fallthru,exec)

  # BLOCK 4
  # PRED: 2 (ab,eh,exec) 3 (ab,eh,exec)
  # result_1(ab) = PHI <result_11(ab)(2), result_12(ab)(3)>
<L0>:;
  <...>

 # BLOCK 5 freq:10000
  # PRED: 3 [100.0%]  (fallthru,exec) 4 [100.0%]  (fallthru,exec)
  # result_2 = PHI <result_12(ab)(3), result_1(ab)(4)>
  return result_2;
  # SUCC: EXIT [100.0%]



and after inlining (*046i.inline) we see:



<bb 2>:

<bb 7>:
  D.2882_11 = request_9(D)->req_pool;
  D.2883_12 = (struct blk *) D.2882_11;
  D.2884_13 = request_9(D)->req_transaction;
  D.2885_14 = VIO_get (tdbb_8(D), rpb_3(D), rsb_1(D), D.2884_13, D.2883_12);

<bb 3>:
  result_15(ab) = (BOOLEAN) D.2885_14;
<...>
  BTR_key (tdbb_8(D), D.2892_22, D.2891_21, D.2890_20, &value, 0B);
  goto <bb 5>;

  # result_24(ab) = PHI <result_10(ab)(7), result_15(ab)(3)>
<L4>:;
<...>

<bb 5>:
  # result_23 = PHI <result_15(ab)(3), result_24(ab)(4)>
  if (result_23 != 0)
    goto <bb 6>;
  else
    goto <bb 7>;

<bb 6>:
  return 1;



Basic block 7 is missing a PHI node which should merge result_23 and result_10.
 The result of this missing PHI should then be used in block 5 instead of
result_10.

The way it currently sits, result 10 is live through basic block 3, which makes
it conflict with result_15, and makes the abnormal edges uncoalescable.

Someone familiar with the inliner can probably fix this quickly. I took a quick
look but its a different world in there :-)
Comment 10 Jakub Jelinek 2007-12-03 18:11:23 UTC
The problem as I see it is that result is uninitialized in the inline routine and in addition to that is SSA_NAME_OCCURS_IN_ABNORMAL_PHI.  After inlining the
empty_stmt SSA_NAME_DEF_STMT is considered to be live from the beginning of the
caller and thus we should add a few PHI nodes around in the caller, if the inline
function is ever called in any kind of loop.
I guess we could detect this e.g. in tree-inline.c (remap_ssa_name), just test for SSA_NAME_OCCURS_IN_ABNORMAL_PHI (name) inside of the if (IS_EMPTY_STMT (SSA_NAME_DEF_STMT (name))) block and if it is (ab) with empty stmt def, add it to some bitmap for later handling.
But I wonder what would be the best way to add the PHI nodes.  We really shouldn't do mark_sym_for_renaming on underlying decl, perhaps create a dummy decl, let intossa create PHIs etc. for it, then change the SSA_NAME_VARs for it? Or anything easier?  Any ideas?
Comment 11 Eric Botcazou 2007-12-16 18:12:52 UTC
Ada testcase at http://gcc.gnu.org/ml/gcc/2007-12/msg00470.html
Comment 12 Jan Hubicka 2007-12-18 11:41:50 UTC
> But I wonder what would be the best way to add the PHI nodes.  We really
> shouldn't do mark_sym_for_renaming on underlying decl, perhaps create a dummy
> decl, let intossa create PHIs etc. for it, then change the SSA_NAME_VARs for
> it? Or anything easier?  Any ideas?

What I hope for is to simply add all possible EH edges (ie edges to all possible receivers not just first one) before inlining, so we never get into busyness of constructing new PHIs because we redirected some EH.
Then we always can use the easy route of copying existing PHI argument from the call site.  I was travelling last weeks, but I will do look into it before Christmas now.

Honza
Comment 13 Jan Hubicka 2007-12-18 18:54:25 UTC
Sorry, my last comment was about different inliner issue that seems to be gone now.  I guess easiest way around would be to initialize to 0 at the beggining of inlined function body?  This happens only for uninitialized SSA registers (otherwise we can do renaming) that should not be at all that common and might result in better code than attempting to preserve the uninitialized values round functions.  I will give it a try.
Comment 14 Jakub Jelinek 2007-12-18 23:31:27 UTC
Zero initialization would work, sure.  I was just worried if it wouldn't result in code pessimization.  The var in the testcase is only unitialized if exception
is thrown, and exceptions should be exceptional.  But the zero initialization will be executed even when no exceptions are thrown.  Couldn't we e.g. zero initialize it only on the EH edges from bbs where it was uninitialized?
Comment 15 Jan Hubicka 2007-12-29 22:31:08 UTC
Well, I would personally not worry that much about pesimization.  The current behaviour of increasing register pressure is worse.  We definitly can insert the initialization to common dominator of all uses of the initialized SSA name, but the dominator tree nor the uses are readily available and I am not convinced it is worth the effort.  I did some behchmarking on C++ benchmarks with patch initializing to 0 and found no regressionons.  I will send patch shortly (the version I tested had little bug causing occasitonal ICEs)
Comment 16 Jan Hubicka 2008-01-03 21:25:12 UTC
Subject: Bug 31081

Author: hubicka
Date: Thu Jan  3 21:23:26 2008
New Revision: 131306

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131306
Log:

	PR tree-optimization/31081
	* tree-inline.c (remap_ssa_name): Initialize uninitialized SSA vars to
	0 when inlining and not inlining to first basic block.
	(remap_decl): When var is initialized to 0, don't set default_def.
	(expand_call_inline): Set entry_bb.
	* tree-inline.h (copy_body_data): Add entry_bb.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-inline.c
    trunk/gcc/tree-inline.h

Comment 17 Richard Biener 2008-01-04 11:16:28 UTC
Fixed.
Comment 18 Richard Biener 2008-01-04 11:17:01 UTC
Subject: Bug 31081

Author: rguenth
Date: Fri Jan  4 11:16:17 2008
New Revision: 131320

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131320
Log:
2008-01-04  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/31081
	* g++.dg/torture/pr31081-1.C: New testcase.
	* g++.dg/torture/pr31081-2.C: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/torture/pr31081-1.C
    trunk/gcc/testsuite/g++.dg/torture/pr31081-2.C
Modified:
    trunk/gcc/testsuite/ChangeLog