On Thu, 2021-03-18 at 09:21 +0800, HAO CHEN GUI wrote: > David & Segher, > > Thanks so much for your explanation. My patch wants to enables the > constant anchor on rs6000 as TARGET_ANCHOR_CONST or targetm.anchor_const > is undefined. I realized that we have addi and addis instructions. So > the range of the offset could be a 32 bit constant. > > I put a test case at > https://github.ibm.com/wschmidt/power-gcc/issues/1042#issuecomment-28922825. > It shows how anchor_const can improve asm output. With anchor_const, the > second complex constant loading can be eliminated by cse if it is within > the range of the first one.
I think about 99.9% of the community won't be able to reach that link. If progress on this issue requires additional eyes on the testcase you may need to provide the test case here. Thanks -Will > > Thanks again and looking forward to your advice. > > On 18/3/2021 上午 8:57, David Edelsohn wrote: > > On Wed, Mar 17, 2021 at 8:26 PM Segher Boessenkool > > <seg...@kernel.crashing.org> wrote: > > > Hi! > > > > > > On Wed, Mar 17, 2021 at 03:35:30PM -0400, David Edelsohn wrote: > > > > I disagree with your new definitions and I disagree with the manner in > > > > which you are trying to change the values. > > > > > > Yes. > > > > > > > Your patch is NOT okay without a lot more explanation and justification. > > > > > > Which is why I said: > > > > > > > > > 1) This isn't suitable for stage 4. > > > > > > You give a lot more reasons to not want it, but that was enough for me. > > > > > > > > > 2) Please add a test case, which shows what it does, that it is > > > > > > useful. > > > > > > I meant there is no way we can accept this patch if we aren't shown what > > > it does, and that that is a good thing. > > > > > > > > > 3) Does this work on other OSes than Linux? What about Darwin and > > > > > > AIX? > > > > > > And here I meant that there is no way we can accept patches that > > > influence code generation on all platforms when we have no idea what it > > > does on most platforms. I did not intend to suggest the patch would be > > > more acceptable if it was tested on other platforms; I wanted to say it > > > is not acceptable if it is not. > > > > > > The main issue is 2). We need to understand what problem this patch is > > > trying to solve. I'm sure Hao Chen had a reason for doing this patch, > > > so I'd like to know what it is trying to achieve, what it is trying to > > > improve! > > > > Investigating this with Segher, I believe that there is some confusion > > about the "ANCHOR" macros. > > > > TARGET_MIN_ANCHOR_OFFSET and TARGET_MAX_ANCHOR_OFFSET are not related > > to TARGET_ANCHOR_CONST. > > > > Also, TARGET_ANCHOR_CONST can be defined as a macro to trigger the > > hook, and doesn't need targetm.anchor_const. > > > > Any change to TARGET_ANCHOR_CONST requires extensive performance > > testing. Yes, it presumably fixes the testcase, but the impact on > > overall performance is the critical question. > > > > Thanks, David
