IPA merge 4: SSA inliner

[Jan Hubicka]

Hi,
thanks for review!
> > 4) SSA operands are copy/constant propagated by inliner.  Inliner always
> >    constant propagated in order to get "const" declared arguments right.
> >    We can easilly do more, but it also tend to uncover latent bugs.
> >    I did little measurements on tramp3d and not doing the constant/copy
> >    prop increases number of SSA names built by 30%, so I think it is 
> >    worthwhile
> >    (and it also makes it easier to inline callbacks)
> >
> I'd like to know what bugs this uncovered.  In the optimizer or the
> inliner?

In the inliner producing invalid gimple by substituting constant
somewhere without properly folding in.  If you check changelogs on
tree-inline for past few months, you can see there was several bugfixes
into this area.  I didn't run across any for a while, so things seems
to've improved, perhaps in longer term a little more sharing with
tree-ccp would help (like we have in RTL backend validate_replace
infrastructure that does replacement right).
> 
> > 5) EH edges comming from inlined function body to landing paths within 
> > outer
> >    function cause troubles since PHI nodes can be associated with the 
> >    landing
> >    pads.  I discussed this with Rth on the summit and we concluded it is 
> >    safe
> >    to to rebuild SSA on those symbols: since they have PHIs on abnormal 
> >    edges
> >    we don't have overlapping liveranges.
> >
> Yes.
> 
> > 6) Problematic are also return values returned via slot.  These variables
> >    do have address taken (technically to pass to inlined function) but we
> >    don't model it properly in the outer function, so they might be in SSA 
> >    form
> >    and we might have issues with partly initialized return values.
> >
> We don't detect that the symbol's address has been taken?  That's not
> good.  Got a test case?

The problem is that the GIMPLE statement looks like:
  struct = fun();
but the semantic of return slot is
  fun(&struct)
with function fun writing result to *struct.  So there is implicit
address taking in the function call.  We don't represent this for alias
analysis and at single function level this would only reduce code
quality.  We know that FUN won't do anything with the address except for
*struct = something...

I can construct testcase later, if you are interested.
> >+ 
> >+   /* Do not set DEF_STMT yet as statement might not get copied.  */
> >+   new = remap_decl (SSA_NAME_VAR (name), id);
> >
> What?  If you are copying the SSA name, why wouldn't you copy the
> statement that defined it?  Is this name going to become a default
> definition?

If we just copied the statement proactively now, we would make one copy
of the statement and when later reaching that statement in copy_bb, we
would make another copy, so the pointer would be wrong.

We initialize proper DEF_STMT in copy_bb.  Perhaps
 /* Do not set DEF_STMT yet as statement is not copied yet.
    Leave the setting to copy_bb.  */
> 
> >+   /* We might've substituted constant or another SSA_NAME for
> >+      the variable.  */
> >+   if ((TREE_CODE (new) == VAR_DECL || TREE_CODE (new) == PARM_DECL)
> >+       /* Replace the SSA name representing RESULT_DECL by variable during
> >+ 	 inlining:  this saves us from need to introduce PHI node in a case
> >+ 	 return value is just partly initialized.  */
> >
> Move this comment before the if().  It's better to have a longer
> description before the whole if() than break appart the flow of the
> predicates with extraneous commentary.  Additionally, I have no idea
> what you were trying to say in that comment.

If you have something like

  fun()
  {
    if (...)
      return 5;
  }

and you inline it into

  ssa_name = fun();

We end up:

  if (...)
    mapped_result_decl = 5;
  ssa_name = mapped_result_decl;

that would be invalid SSA form if mapped_result_decl was SSA_NAME.  I
simply force it to be a variable.
> 
> >+       && (TREE_CODE (SSA_NAME_VAR (name)) != RESULT_DECL
> >+ 	  || !id->transform_return_to_modify))
> >+     {
> >+       new = make_ssa_name (new, NULL);
> >+       insert_decl_map (id, name, new);
> >+       if (IS_EMPTY_STMT (SSA_NAME_DEF_STMT (name))
> >+ 	  /* When inlining, parameters are replaced by initialized vars.  */
> >+ 	  && (TREE_CODE (new) == PARM_DECL || TREE_CODE (name) != PARM_DECL))
> >
> How could 'name' be anything other than an SSA_NAME?  '|| TREE_CODE
> (name) != PARM_DECL' is trivially true.

Yep, looks like some thinko I made long time ago.  
> >+ 	      && (def = gimple_default_def (id->src_cfun, decl)))
> >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> SSA_NAME_IS_DEFAULT_DEF (def)
OK,
it is also initializing the "def", but I can definitly clean up ;)
> >
> So, you set DEF statement for all the remapped names here?  If so,
> then I'm not so concerned about setting DEF_STMT to NULL in
> remap_ssa_name.

Yes :)
> 
> >+    This might change in future if we allow redirecting of EH edges and
> >+    we might want to change way build CFG pre-inlining to include
> >+    all the possible edges then.  */
> >
> You don't seem to be documenting what the function *does*.
> Documentation for BB is also missing.

It does precisely what says the beggining of last paragraph "It is safe
to..."
I will change it "The function does... it is safe because..."
> >+       if (gimple_in_ssa_p (cfun))
> >+         mark_new_vars_to_rename (copy_stmt);
> >
> Careful here.  This has disappeared.  You *could* use
> mark_symbols_for_renaming, but that only works if the symbols you want
> to rename are not wrapped in SSA names in the operands.  What symbols
> are you trying to mark for renaming here?

I want to basically build VOPS for static vairables and SSA for
variables where I decided to strip down the SSA_NAME for reasons
discussed above.
> >+      We might want to introduce a notion that single SSA_NAME might
> >+      represent multiple variables for purposes of debugging. */
> >
> No.  Lord, no.

If you have multiple source level vars sotred at one place, you need it
if we want to be right.  It does not mean multiple VAR_DECLs, more
something like REG attributes we have in RTL, but that is not important
now.  Just wanted to drop note that this is particularly nasty for
debugging, I usually want to be able to print functio nargument ;)
> 
> >+   if (gimple_in_ssa_p (cfun) && rhs && def && is_gimple_reg (p)
> >+       && (TREE_CODE (rhs) == SSA_NAME
> >+ 	  /* Replacing &""[0] has interesting side effects.  Exclude 
> >ADDR_EXPRs
> >+ 	     now.  */
> >
> What are those side effects?  Again, I'd rather have this comment
> before the if().  Though, I guess I can't win this argument as I see
> lots of instances like this one sprinkled all over the place.

The operand has to be folded into canonical form that is not what
inliner did at the time I was writting the code.  There was number of
improvements in this area, so perhaps this is no longer needed.

We can drop it from initial patch and I will double check how things
behave now before enabling inliner on SSA as this is executed multiple
times in fortran testsuite.

The idea would be to get basic stuff here and then i will post fixes to
such a side corners with more detailed descriptions. OK?
> 
> >! 	  def = remap_ssa_name (def, id);
> >!           init_stmt = build2 (MODIFY_EXPR, TREE_TYPE (var), def, rhs);
> >
> You mean GIMPLE_MODIFY_STMT here.

Definitly! The patch was prepared before ;)
> 
> >! 	      /* FIXME: rewriting random variables in SSA form is going
> >! 		 to cause missoptimizations once we start optimizing. 
> >! 
> I don't understand this.  Why would we have mis-optimizations?  Show
> me sample code?

When you have overlapping life range:

You get someting like


  vara_version_1 = fun ();
  vara_version_2 = fun ();

then we execute the trick of stripping SSA form described above and we
get

  vara = fun ();
  vara = fun ();

and execute renamer.  Perhaps this is another ugly case we can drop from
initial patch and discuss fully incrementally?  (It is needed for C++,
but for C we don't do return slots). 

Thanks,
Honza