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
