Hi! On Mon, Mar 06, 2023 at 07:13:08PM +0000, Richard Sandiford wrote: > Segher Boessenkool <seg...@kernel.crashing.org> writes: > > Most importantly, what makes you think this is a problem for aarch64 > > only? If it actually is, you can fix it in the aarch64 config! Either > > with or without new hooks, whatever works best. > > The point is that I don't think it's a problem for AArch64 only. > I think it's a generic issue that should be solved in a generic way > (which is what the patch is trying to do). The suggestion to restrict > it to AArch64 came from Jakub. > > The reason I'm pushing back against a hook is precisely because > I don't want to solve this in AArch64-specific code.
But it is many times worse still to do it in target-specific magic code disguised as generic code :-( If there is no clear explanation why combine should do X, then it probably should not. > I'm not sure we would be talking about restricting this to AArch64 > if the patch had been posted in stage 1. If people are concerned > about doing this for all targets in stage 4 (which they seem to be), Not me, not in principle. But it takes more time than we have left in stage 4 to handle this, even for only combine. We should give the other target maintainers much longer as well. > I thought the #ifdef was the simplest way of addressing that concern. An #ifdef is a way of making a change that is not finished yet not hurt the other targets. It still hurts generic development, which indirectly hurts all targets. > And I don't think what the patch does is ad hoc. It is almost impossible to explain what it does and why that is a good thing, why it is what we want, what we should do here; and certainly not in a compact, terse, focused way. It has all the hallmarks of ad hoc patches. > Reorganising the > expression in this way isn't something new. extract_left_shift already > does a similar thing (and does it for all targets). That is not similar at all, no. /* See if X (of mode MODE) contains an ASHIFT of COUNT or more bits that can be commuted with any other operations in X. Return X without that shift if so. */ If you can factor out a utility function like that, with an actual nice description like that, it would be a much more palatable patch. Segher
