[Bug tree-optimization/114151] [14 Regression] weird and inefficient codegen and addressing modes since r14-9193

["rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Andrew Macleod from comment #7)
> (In reply to Richard Biener from comment #6)
> > (In reply to Andrew Macleod from comment #5)
> > > (In reply to rguenther@suse.de from comment #4)
> > > 
> > > > 
> > > > What was definitely missing is consideration of POLY_INT_CSTs (and
> > > > variable polys, as I think there's no range info for those).
> > > > 
> > > Ranger doesn't do anything with POLY_INTs, mostly because I didn't
> > > understand them.  
> > > 
> > > > We do eventually want to improve how ranger behaves here.  I'm not sure
> > > > why when we do not provide a context 'stmt' it can't see to compute
> > > > a range valid at the SSA names point of definition?  (so basically
> > > > compute the global range)
> > > 
> > > The call looks like it doesn't provide the stmt.  Without the stmt, all
> > > ranger will ever provide is global ranges.
> > > 
> > > I think you are asking why, If there is no global range, it doesn't try to
> > > compute one from the ssa_name_def_stmt?  Ranger does when it is active.  
> > 
> > I tried with an active ranger but that doesn't make a difference.  Basically
> > I added enable_ranger () / disable_ranger () around the pass and thought
> > that would "activate" it.  But looking at range_for_expr I don't see how
> > that would make a difference without a provided stmt.
> > 
> 
> It wouldn't. why isn't a context stmt being provided?

I don't have one in this context.  There supposedly is one, but it's not
passed down, I'm also not sure we can use that since it would taint SCEV
info with possibly context sensitive info that's re-used (by cache) in
other context.

> range_of_expr with no context stmt makes no attempt to calculate anything.
> This is because one can get into a lot of trouble as it doesn't know whether
> the expression you are calculating is even in the IL or just some detached
> tree expression.
> 
> If you have an SSA NAME and want to actually calculate the value, you can
> use range_of_stmt (range, SSA_NAME_DEF_STMT (name))  instead of
> range_of_expr ().

Yes, the issue is that I might have a GENERIC expression like _1 + 2, or
even _1 + _2.

> If you pass in a stmt as context, and the SSA_NAME you are asking about is
> the LHS of the stmt, then range_of_expr will call range_of_stmt itself...
> but again, it needs a context stmt in order to know its safe to do so.

But for an SSA name it should be always safe to assume its definition as
context because that's effectively what it's global range is computed from.
If it's not in the IL then gimple_bb should be NULL, so this could be
guarded against ...

> In general, range_of_expr with no context will not calculate anything...
> When a stmt for location context is provided, then its free to go an do
> whatever calculations are required.

So I'd like to see global ranges computed on-demand here since I can't
easily pass in the definition stmt for a GENERIC expression.  Sth like
(untested)

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index c16b776c1e3..2f481695c22 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -98,33 +98,35 @@ gimple_ranger::range_of_expr (vrange &r, tree expr, gimple
*stmt)
        fputs ("\n", dump_file);
     }

-  // If there is no statement, just get the global value.
-  if (!stmt)
-    {
-      Value_Range tmp (TREE_TYPE (expr));
-      m_cache.get_global_range (r, expr);
-      // Pick up implied context information from the on-entry cache
-      // if current_bb is set.  Do not attempt any new calculations.
-      if (current_bb && m_cache.block_range (tmp, current_bb, expr, false))
-       {
-         r.intersect (tmp);
-         char str[80];
-         sprintf (str, "picked up range from bb %d\n",current_bb->index);
-         if (idx)
-           tracer.print (idx, str);
-       }
-    }
   // For a debug stmt, pick the best value currently available, do not
   // trigger new value calculations.  PR 100781.
-  else if (is_gimple_debug (stmt))
+  if (stmt && is_gimple_debug (stmt))
     m_cache.range_of_expr (r, expr, stmt);
   else
     {
-      basic_block bb = gimple_bb (stmt);
+      basic_block bb = stmt ? gimple_bb (stmt) : NULL;
       gimple *def_stmt = SSA_NAME_DEF_STMT (expr);

+      // If the definition is not in the IL or this is a default def and
+      // there's no context, just get the global value.
+      if ((!stmt && SSA_NAME_IS_DEFAULT_DEF (expr))
+         || (!SSA_NAME_IS_DEFAULT_DEF (expr) && !gimple_bb (def_stmt)))
+       {
+         Value_Range tmp (TREE_TYPE (expr));
+         m_cache.get_global_range (r, expr);
+         // Pick up implied context information from the on-entry cache
+         // if current_bb is set.  Do not attempt any new calculations.
+         if (current_bb && m_cache.block_range (tmp, current_bb, expr, false))
+           {
+             r.intersect (tmp);
+             char str[80];
+             sprintf (str, "picked up range from bb %d\n",current_bb->index);
+             if (idx)
+               tracer.print (idx, str);
+           }
+       }
       // If name is defined in this block, try to get an range from S.
-      if (def_stmt && gimple_bb (def_stmt) == bb)
+      else if (gimple_bb (def_stmt) == bb)
        {
          // Declared in this block, if it has a global set, check for an
          // override from a block walk, otherwise calculate it.

I thought about adding a flag or using a magic (gimple *)-1 stmt argument
but I can't see where computing new global ranges could go wrong
(well, OK, I guess I can - but I wonder whether affected passes wouldn't
only have the global_range_query anyway).
So passing NULL should be similar behavior
as when passing in a debug stmt - we only query the cache, I wonder
whether calling that 'cached_range_of_expr' might be easier?  Or
calling what I like range_of_def (but make it work on GENERIC as well)?
At least the defaulted NULL stmt in range_of_expr makes it difficult to
extent that API.