Hao, Segher and I do not doubt that the patch can improve the examples and testcases. The question is if those examples are representative of common situations and if the patch truly improves performance overall -- for real workloads. Can you test the performance impact of your patch, not only demonstrating the change in code generation?
Also, I understand about the range of constants that you wish to address, but TARGET_MIN_ANCHOR_OFFSET (targetm.min_anchor_offset) and TARGET_MAX_ANCHOR_OFFSET (targetm.max_anchor_offset) are parameters for completely different features in GCC. I realize that some GCC ports define the values in close proximity in the source files, but your patch to define TARGET_ANCHOR_CONST should not define or change the other macros. The patch only should define TARGET_ANCHOR_CONST. And I believe that it should be possible to define TARGET_ANCHOR_CONST in rs6000.c #define TARGET_ANCHOR_CONST 0x8000 instead of using targetm.anchor_const, which would be more consistent with the style for definitions of other values in the rs6000 port. Thanks, David On Wed, Mar 17, 2021 at 9:21 PM HAO CHEN GUI <guih...@linux.ibm.com> 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. > > 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
