Re: [PATCH] Improve handling of register attributes--fix PA indexedstore problem

[Jim Wilson <wilson at specifixinc dot com>]

John David Anglin wrote:
> The first part of the patch improves the handling of register
> attributes and the register pointer flag.  This is necessary to
> allow unscaled indexed modes to be created during the initial
> RTL generation on the PA and to ensure that the created
> instructions will still be recognizable after CSE.

I think this too large and too invasive of a patch to try to install at 
this point in the gcc-3.4 release process.  Also, given the Richard 
Sandiford has reverted the reload patch that created the problem you are 
 trying to solve here, we no longer need the patch in gcc-3.4.

I find it hard to understand why all of the fuss about REG_ATTRS here. 
They don't seem to be of much use.  It looks like most of the code for 
them is just copying them from one register to another.  MEM_ATTRS are 
used in alias, and occasionally a REG_ATTR gets copied into a MEM_ATTR 
which does something useful.  Otherwise they seem to be a waste.  All 
you really care about is REG_POINTER which isn't in REG_ATTR.  Do you 
really need all of this to keep REG_POINTER correct?  Just remembering 
to copy REG_POINTER from old regs to new regs would be simpler.  It does 
seem that you can get some additional accuracy by using REG_ATTR though, 
and as a side benefit, we may be getting better alias analysis because 
of this.

I'm concerned that you are changing a common interface in a non-obvious 
way.  gen_reg_rtx (GET_MODE (reg)) is very common.  People are going to 
forget to use gen_reg_rtx_set_attrs instead.  If you rely heavily on 
this, you may end up in a situation where the PA port is frequently 
broken.  There is also the issue that we have code on branches, rtlopt, 
hammer, tree-ssa, and there is no obvious way to make sure that they get 
fixed, which means the PA port will break when code is merged in from 
those branches.  If we are going to change a key interface, like 
gen_reg_rtx, it helps to do it in a way that ensures old code won't 
compile.  But since many uses of gen_reg_rtx are still OK, it isn't 
clear how to do that.  There is also the problem that if target backends 
will need to be fixed to take full advantage of this, then this is a big 
job that will never be finished.  This probably only matters for targets 
that use REG_POINTER though, of which there probably aren't many.

There seems to be some redundant setting of REG_POINTER.  See for 
instance the web.c change.  You call gen_reg_rtx_set_attrs, which calls 
set_reg_ptr_and_attrs which checks REG_POINTER of the old reg, and if 
set calls mark_reg_pointer to set it on the new reg.  Then after 
returning to web.c, we copy REG_POINTER from the old reg to the new one. 
 web.c seems to be the only place that does this though.

I don't see what the print-rtl.c change is for.  The code seems obtuse. 
 Your text says it has something to do with the INDIRECT_REF change in 
but doesn't say exactly what.  print_mem_expr already has code for 
INDIRECT_REF, so I don't see how you can get something that isn't a decl 
by the time we get to the DECL_NAME check.  Apparently we can't use 
DECL_P because the gen* programs don't have tree.def.  I had to think 
about that one, it wasn't immediately obvious to me.

I didn't try to review the PA changes.  You can do that yourself.

I don't see anything really wrong with the patch other than the 
print-rtl.c change, and an 3-8% speedup is definitely something we want. 
 So I am willing to go ahead with this.

I'm wondering is maybe some of the speedup is due to aliasing 
improvements as a side-effect of your changes.  If so, this might help 
other targets.  This may also introduce bugs into other targets if it 
exposes latent aliasing bugs.

So, to summarize,
1) I think you are risking destabilizing the PA port with this, but that 
is your problem.
2) I don't like the print-rtl.c change.  I'd like a better explanation 
of what the problem is here.
3) With the print-rtl.c change resolved, I think this is OK after the 
gcc-3.4 release branch is made.
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com