public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Eric Botcazou <ebotcazou@adacore.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [patch] Perform anonymous constant propagation during inlining
Date: Fri, 01 May 2015 10:29:00 -0000	[thread overview]
Message-ID: <2489231.jUt36nmj7L@polaris> (raw)
In-Reply-To: <CAFiYyc2DK8P5pEk2EcrsubZtquYx6ZoJPVAWva1pqqXk0hOQoA@mail.gmail.com>

> Hmm, special-casing this in the inliner looks odd to me.  ISTR the inliner
> already propagates constant parameters to immediate uses, so I guess
> you run into the casting case you handle specially.

Right on both counts, the original GIMPLE looks like:

  right.3 = (system__storage_elements__integer_address) right;
  D.4203 = D.4201 % right.3;
  D.4200 = (system__storage_elements__storage_offset) D.4203;
  return D.4200;

and setup_one_parameter has:

  /* If there is no setup required and we are in SSA, take the easy route
     replacing all SSA names representing the function parameter by the
     SSA name passed to function.

     We need to construct map for the variable anyway as it might be used
     in different SSA names when parameter is set in function.

     Do replacement at -O0 for const arguments replaced by constant.
     This is important for builtin_constant_p and other construct requiring
     constant argument to be visible in inlined function body.  */
  if (gimple_in_ssa_p (cfun) && rhs && def && is_gimple_reg (p)
      && (optimize
          || (TREE_READONLY (p)
	      && is_gimple_min_invariant (rhs)))
      && (TREE_CODE (rhs) == SSA_NAME
	  || is_gimple_min_invariant (rhs))
      && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (def))
    {
      insert_decl_map (id, def, rhs);
      return insert_init_debug_bind (id, bb, var, rhs, NULL);
    }

and here is the GIMPLE after inlining:

  _17 = _16;
  _18 = _17;
  right.3_19 = 8;
  _20 = _18 % right.3_19;
  _21 = (system__storage_elements__storage_offset) _20;

so the constant replacement is done for right.3_19 by the above block.

> But then if any constant propagation is ok at -O0 why not perform
> full-fledged constant propagation (with !DECL_IGNORED_P vars as a
> propagation barrier) as a regular SSA pass?  That is, if you'd had a
> Inline_Always function like
> 
> int foo (int i)
> {
>   return (i + 1) / i;
> }
> 
> you'd not get the desired result as the i + 1 stmt wouldn't be folded
> and thus the (i + 1) / i stmt wouldn't either.

That's OK, only the trivial cases need to be dealt with since it's -O0 so 
running a fully-fledged constant propagation seems overkill.

> That said - why does RTL optimization not save you here anyway?
> After all, it is responsible for turning divisions into shifts.

You mean the RTL expander?  Because the SSA name isn't replaced with the RHS 
of its defining statement in:

      /* For EXPAND_INITIALIZER try harder to get something simpler.  */
      if (g == NULL
	  && modifier == EXPAND_INITIALIZER
	  && !SSA_NAME_IS_DEFAULT_DEF (exp)
	  && (optimize || DECL_IGNORED_P (SSA_NAME_VAR (exp)))
	  && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp)))
	g = SSA_NAME_DEF_STMT (exp);

since modifier is EXPAND_NORMAL.  Do you think it would be OK to be a little 
more aggressive here?  Something like:

      /* If we aren't on the LHS, look into the defining statement.  */
      if (g == NULL
	  && modifier != EXPAND_WRITE
	  && !SSA_NAME_IS_DEFAULT_DEF (exp)
	  && (optimize || DECL_IGNORED_P (SSA_NAME_VAR (exp)))
	  && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp)))
	 {
	    g = SSA_NAME_DEF_STMT (exp);
	    /* For EXPAND_INITIALIZER substitute in all cases, otherwise do
	       it only for constants.  */
	    if (modifier != EXPAND_INITIALIZER
		&& (!gimple_assign_copy_p (g)
		    || !is_gimple_min_invariant (gimple_assign_rhs1 (g))))
	      g = NULL;
	 }

That's certainly sufficient here.

-- 
Eric Botcazou

  reply	other threads:[~2015-05-01 10:29 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-29 10:23 Eric Botcazou
2015-04-29 12:06 ` Richard Biener
2015-05-01 10:29   ` Eric Botcazou [this message]
2015-05-01 14:19     ` Richard Biener
2015-05-01 16:44       ` Eric Botcazou
2015-05-01 18:11         ` Eric Botcazou
2015-05-04  8:46           ` Richard Biener
2015-05-04  9:32             ` Eric Botcazou
2015-05-04 21:40           ` Eric Botcazou
2015-05-05  5:43             ` Richard Biener
2015-05-11 14:07               ` Eric Botcazou
2015-05-12  8:46                 ` Richard Biener
2015-04-29 13:29 ` Jan Hubicka
2015-04-29 13:50   ` Richard Biener
2015-04-29 14:31     ` Jan Hubicka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2489231.jUt36nmj7L@polaris \
    --to=ebotcazou@adacore.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=richard.guenther@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).