On 09/22/14 00:40, Ilya Enkovich wrote:
Bounds don't have to vary for different pointers. E.g. p and p + 1
always have equal bounds. In this particular case we have function
pointers and all of them have default bounds.
OK. It looked a bit odd and I wanted to make sure there wasn't
something fundamentally wrong.
I attach a dump I got from Chrome compilation with no additional
checks restrictions in split. Original function returns value defined
by phi node in return_bb and bounds defined in BB2. Split part
contains BB3, BB4 and BB5 and resulting function part has usage of
returned bounds but no producer for it.
Right, but my question is whether or not the bounds from BB2 were really the
correct bounds to be using in the first place! I would have expected a PHI
in BB6 to select the bounds based on the path leading to BB6, much like we
select a different return value.
Consider we have pointer computation and then
return __bnd_init_ptr_bounds (res);
In such case you would never have a PHI node for bounds. Also do not
forget that we may have no PHI nodes for both return value and return
bounds. In such case we could also easily fall into undefined value
as in dump.
This code (visit_bb, find_return_bb, consider_split) is a bit of a mess,
but I do see what you're trying to do now. Thanks for being patient
with my questions.
If I were to look at this at a high level, the core issue seems to me
that we're really not prepared to handle functions with multiple return
values. This shows up in your MPX work, but IIRC there's cases in the
atomics where we have multiple return values as well. I wouldn't be
surprised if there's latent bugs with splitting & atomics lurking to
bite us one day.
So if I'm reading all this code correctly, given a return block which
returns a (pointer,bounds) pair, if the bounds are set by a normal
statement (ie, not a PHI), then we won't use that block for RETURN_BB.
So there's nothing to worry about in that case. Similarly if the
bounds are set by a PHI in the return block, consider_split will reject
that split point as well. So really the only case here is when the
bounds are set in another dominating block. Right?
I can see how you're using the relevant part of the same test we need
for the retval. My gut tells me we want to commonize that test so that
they don't get out-of-sync. Specifically, can we pull the code which
sets split_part_set_retbnd into a little function, then use it for the
retval here too:
else if (TREE_CODE (retval) == SSA_NAME)
current->split_part_set_retval
= (!SSA_NAME_IS_DEFAULT_DEF (retval)
&& (bitmap_bit_p (current->split_bbs,
gimple_bb (SSA_NAME_DEF_STMT (retval))->index)
|| gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb));
Iteration through the statements in find_retbnd should start at the end
of the block and walk backwards. It probably doesn't matter in practice
all that much, but might as well be sensible since the GIMPLE_RETURN is
almost always going to be the last statement in the block.
Similarly for the statement walk in split_function when you want to
replace retbnd with new one.
It seems like the code to build the bndret call to obtain bounds is
repeated. Can you refactor that into its own little function and just
use that. It's not a huge amount of code, but it does make things a bit
easier to follow.
With those changes this will be OK.
Jeff
