Andrew Pinski <pins...@gmail.com> writes: > On Fri, Jun 20, 2025, 4:47 PM Wilco Dijkstra <wilco.dijks...@arm.com> wrote: > >> >> TARGET_CONST_ANCHOR appears to trigger too often, even on simple >> immediates. >> It inserts extra ADD/SUB instructions even when a single MOV exists. >> Disable it to improve overall code quality: on SPEC2017 it removes >> 1850 ADD/SUB instructions and 630 spill instructions, and SPECINT is ~0.06% >> faster on Neoverse V2. Adjust a testcase that was confusing neg and fneg. >> >> Passes regress, OK for commit? >> > > I am not sure 2017 might be best benchmark for this. > Do you have more data? > Even on which spill is happening where? > > This seems like it would be better to improve the RA rather than workaround > it via this. > > Also 0.6% is in the noise as far as I know.
0.06% :-) (Just saying because 0.6% sounded pretty decent.) But yeah, I agree that 0.06% isn't very compelling on its own. I'm not saying we shouldn't do this. But I don't think the message above makes the case. From a review perspective, it doesn't sound much different from "Pull random levers and see what happens. If a lever gives slightly better results in one position than another, then keep the lever in that position." And while that might be appropriate for some things, I'm not sure it's a good approach to take for RA-related issues. It's very easy for innocuous changes in one part of the code to trigger spills elsewhere. When it comes to spilling, numbers on their own don't separate the root cause from incidental effects. And like you say, it isn't clear from the results presented whether this is a broad-based improvement or just one or two workloads. The numbers also don't give the total size delta. So IMO we should at least look at what the RA is doing with the current code, what spilling decisions it's making, and whether those decisions are reasonable in context. That way we can tell whether this is a systemic issue caused directly by the target hook and whether removing the hook is a reasonable thing to do. Also "It inserts extra ADD/SUB instructions even when a single MOV exists." isn't necessarily a fault with the hook, but could be an issue elsewhere in the RTL pipeline. So there too, I think we should look at what's happening, rather than go straight to removing the hook. Thanks, Richard
