Re: [PR64164] drop copyrename, integrate into expand

[Alexandre Oliva <aoliva at redhat dot com>]

On Mar 31, 2015, Jeff Law <law@redhat.com> wrote:

>> -  if (var1 != var2)
>> +  if (var1 != var2 && !optimize)
>> return false;
> So when when the base variables are different and we are optimizing,
> this allows coalescing, right?

Yeah.

> What I don't see is a corresponding change to var_map_base_init to
> ensure we build a conflict graph which includes objects when
> SSA_NAME_VARs are not the same.  I see a vague reference in
> var_map_base_init's header comment that refers us to
> coalesce_ssa_name.

> It appears that compute_optimized_partition_bases handles this by
> creating a partitions of things that are related by copies/phis
> regardless of their underlying named object, type, etc.  Right?

Correct.  I guess it makes sense to move partition base computation to a
single location.  Since compute_optimized_partition_bases relies on data
structures local to this source file, I'm moving the non-optimized
version to tree-ssa-coalesce.c, and dropping support for basevar
initialization from tree-ssa-live.c.


> Hard to argue with removing a pass that gets called 5 times!

:-)


>> @@ -890,6 +900,36 @@ build_ssa_conflict_graph (tree_live_info_p liveinfo)
>> live_track_process_def (live, result, graph);
>> }
>> 
>> +      /* Pretend there are defs for params' default defs at the start
>> +	 of the (post-)entry block.  We run after abnormal coalescing,
>> +	 so we can't assume the leader variable is the default
>> +	 definition, but because of SSA_NAME_VAR adjustments in
>> +	 attempt_coalesce, we can assume that if there is any
>> +	 PARM_DECL in the partition, it will be the leader's
>> +	 SSA_NAME_VAR.  */

This comment is outdated.  Since we no longer have abnormal coalescing
before building the conflict graph, we can just test whether each
SSA_NAME is a default def for a PARM_DECL and be done with it.

> So the issue here is you want to iterate over the objects live at the
> entry block, which would include any SSA_NAMEs which result from
> PARM_DECLs.  I don't guess there's an easier way to do that other than
> iterating over everything live in that initial block?

We could iterate over all SSA_NAMEs, but that would probably be more
costly.  There shouldn't be very many live variables at the function
entry, so using the live bitmaps is likely to save us time, especially
on functions with lots of SSA_NAMEs.

> And the second second EXECUTE_IF_SET_IN_BITMAP iterates over
> everything in the partitions associated with the SSA_NAMES that are
> live at the the entry block, right?

Yeah, we iterate over the bases in live_base_var, because the per-base
bitmaps are only accurate when the corresponding live_base_var bit is
iset.

>> @@ -1126,11 +1166,12 @@ create_outofssa_var_map (coalesce_list_p cl, bitmap used_in_copy)
>> 
>> static inline bool
>> attempt_coalesce (var_map map, ssa_conflicts_p graph, int x, int y,
>> -		  FILE *debug)
>> +		  bitmap param_defaults, FILE *debug)
> [ ... ]
> So the bulk of the changes into this routine are really about picking
> a good leader, which presumably is how we're able to get the desired
> effects on debuginfo that we used to get from tree-ssa-copyrename.c?

This has nothing to do with debuginfo, I'm afraid.  We just had to keep
track of parm and result decls to avoid coalescing them together, and
parm decl default defs to promote them to leaders, because expand copies
incoming REGs to pseudos in PARM_DECL's DECL_RTL.  We should fill that
in with the RTL created for the default def for the PARM_DECL.  At the
end, I believe we also copy the RESULT_DECL DECL_RTL to the actual
return register or rtl.  I didn't want to tackle the reworking of these
expanders to avoid problems out of copying incoming parms to one pseudo
and then reading it from another, as I observed before I made this
change.  I'm now tackling that, so that we can refrain from touching the
base vars altogether, and not refrain from coalescing parms and results.

> So some comments about the various cases here might help.  I can sort
> them out if I read the code, but one could argue that a block comment
> on the rules for how to select the partition leader would be better.

*nod*.  I won't bother, though, if this code ends up gone in the next
iteration of the patch ;-)

> Is the special casing of PARM_DECLs + RESULT_DECLs really a failing of
> not handling one or both properly when computing liveness information?

No, it's about RTL assignment and copying to/from hard regs.  We assign
RTL to PARM_DECLs and RESULT_DECLs explicitly in the expander, but we
can't assign different RTL to them if they are coalesced in a single
partition.

> I'm not aware of an inherent reason why a PARM_DECL couldn't coalesce
> with a related RESULT_DECL if they are otherwise non-conflicting and
> related by a copy/phi.

Indeed, there isn't any inherent reason.  It was just a restriction I
carried over from copyrename, and that I postponed cleaning up.

> Presumably ordering of unioning of the partitions doesn't matter here
> as we're looking at coalesce possibilities rather than things we have
> actually coalesced?  Thus it's OK (?) to handle the names occurring in
> abnormal PHIs after those names that are associated by a copy.

Yeah, they'll end up with the same basevar one way or another.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer