On 10/5/20 8:18 PM, Andrew MacLeod wrote:
On 10/5/20 4:16 PM, Martin Sebor wrote:
On 10/5/20 8:50 AM, Aldy Hernandez via Gcc-patches wrote:
[Martin, as the original author of this pass, do you have any concerns?]
@@ -1270,7 +1271,21 @@ get_size_range (tree exp, tree range[2], bool
allow_zero /* = false */)
enum value_range_kind range_type;
if (integral)
- range_type = determine_value_range (exp, &min, &max);
+ {
+ if (query)
+ {
+ value_range vr;
+ gcc_assert (TREE_CODE (exp) == SSA_NAME
+ || TREE_CODE (exp) == INTEGER_CST);
+ gcc_assert (query->range_of_expr (vr, exp, stmt));
Will the call to the function in the assert above not be eliminated
if the assert is turned into a no-op? If it can't happen (it looks
like it shouldn't anymore), it would still be nice to break it out
of the macro. Those of us used to the semantics of the C standard
assert macro might wonder.
gcc_assert contents will not be eliminated in a release compiler, only
the actual check of the return value. The body of the assert will
continue to be executed.
This exists because if we were to try to check the return value, we'd
have to do something like
bool ret = range_of_expr (...)
gcc_checking_assert (ret);
and when the checking assert goes away, we're left with an unused
variable 'ret' warning. the gcc_assert () resolves that issue.
Right. The unused warning for the variable would of course have to
be avoided. There are several ways of doing that we're all familiar
with. What I wasn't sure about and had to go out of my way to
convince myself of is that the gcc_assert() argument isn't eliminated
when checking is disabled. There's still a definition of gcc_assert
in gcc/system.h where it is eliminated, but that definition is never
used). I think the code should be changed so that others (or me if
I forget) don't wonder the same thing in the future. Another
possibility is to add a comment reassuring the reader that
the argument is always evaluated.
I'm not a huge fan of them, but they do serve a purpose and seem better
than the alternatives :-P
The first assert should however be a gcc_checking_assert since its just
a check.. and then that will go away in a release compiler.
-/* Execute the pass for function FUN, walking in dominator order. */
-
unsigned
pass_wrestrict::execute (function *fun)
{
- calculate_dominance_info (CDI_DOMINATORS);
-
- wrestrict_dom_walker walker;
- walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
+ gimple_ranger ranger;
+ basic_block bb;
+ FOR_EACH_BB_FN (bb, fun)
+ wrestrict_walk (&ranger, bb);
return 0;
}
@@ -159,11 +144,14 @@ public:
only the destination reference is. */
bool strbounded_p;
- builtin_memref (tree, tree);
+ builtin_memref (range_query *, gimple *, tree, tree);
tree offset_out_of_bounds (int, offset_int[3]) const;
private:
+ gimple *stmt;
+
+ range_query *query;
Also please add a comment above STMT to make it clear it's the call
statement to the builtin.
For QUERY, I'm not sure adding a member to every class that needs
to compute ranges is the right design. At the same time, adding
an argument to every function that computes ranges isn't great
either. It seems like there should be one shared ranger instance
that could be used by all clients/passes as well as untility
functions called from them. It could be a global object set/pushed
by each pass when it starts and popped when it ends, and managed by
the ranger API. Say a static member function:
range_query* range_query::instance ();
range_query* range_query::push_instance (range_query*);
range_query* range_query::pop_instance ();
As some background, when I wrote the builtin_access access
I envisioned using it as a general-purpose class in other similar
contexts. That hasn't quite happened yet but there is a class
kind of like it that might eventually end up taking the place of
builtin_access. It's access_ref in builtins.h. And while neither
class crates a lot of instances so far, I'm about to post a patch
that does create one or two instances of access_ref per SSA_NAME
of pointer type. Having an extra member in each instance just
to gain access to an API would be excessive.
I'm not saying all this as an objection to the change but more
as something to think about going forward.
Long term, I would expect we might have some sort of general access...
probably thru cfun. so any pass/routines would just ask for
RANGE_INFO (cfun)->range_of_expr()
The default would be a general value_range implementation which probably
implements something like determine_value_range_1 ().. and if a pass
wants to use a ranger, then it could register a ranger, and when its
done delete it. and it would just work for everyone everywhere.
That would work too.
As a side note, I don't yet fully understand when it might be useful
to have different range_query instances. We talked about value_query
and range_query but those are provided because some passes work with
just values and others with just ranges. When might one want to have
an instance of some other type altogether and tie it to a function?
but we're not there yet, and we havent worked out the details :-) for
the moment, passes need to figure out themselves how to access the
ranger they create if they want one. They had to manage a
range_analyzer before if the used anything other than global ranges, so
that is nothing new.
For EVRP, yes. For everything else there's the global get_range_info.
It got ugly when a utility function like get_size_range was changed
to work with both. I made that change and didn't like it then. I'm
trying to brainstorm ways to avoid spreading that wart too much
further.
Martin
