This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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,

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]