"H.J. Lu" <[email protected]> writes: > I put users/hjl/pr124798/master branch at > > https://gitlab.com/x86-gcc/gcc/-/commits/users/hjl/pr124798/master > > I applied all of your patches and merge some of them into > > commit 9d0043fdcbab21ed2cc3ac547fe4b37c6f02ac94 > Author: H.J. Lu <[email protected]> > Date: Tue Apr 14 18:37:20 2026 +0800 > > x86: Implement TARGET_FNTYPE_ABI > > with these changes:
Thanks. The updates mostly look good to me FWIW. I noticed after posting that I'd failed to remove the final loop in ix86_conditional_register_usage, so thanks for catching that. On: > @@ -21842,15 +21869,32 @@ ix86_initialize_abi (unsigned int abi_id, > HARD_REG_SET full_reg_clobbers) > } > > /* Return the descriptor of no_callee_saved_registers function type. > - None of the enabled registers are preserved, except for the common > - rules applied by ix86_initialize_abi. */ > + None of the registers are preserved, except for frame register to > + mitigate PR target/114116. The reason I changed this is that I don't think the frame pointer handling is to mitigate that PR, at least not any more. That PR did not make callers assume that no_callee_saved_registers callees would preserve the frame pointer. The fix just made a local decision within the callee itself. And the fix added TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP to represent that local decision. It's always OK for a callee to save a register that it doesn't "need" to. :) That can sometimes be useful as a QoI thing. The more relevant PR seems to be PR120840, which established that callers *can* assume that the frame pointer is preserved (at least by toolchains that have the fix). preserve_none is now a public, cross-toolchain ABI. The de facto specification seems to be that it preserves the frame pointer. That's what PR120840 implemented for GCC and is what LLVM seems to believe [https://godbolt.org/z/KrGePj3or]. Even if GCC wanted to take a different approach to PR114116 in future, it wouldn't be able to change the frame pointer behaviour without an ABI break with LLVM and with GCC 16. In summary: if we want to remove the frame pointer here, it has to be a guarantee made by the specification. It can't just be a mitigation. Removing the frame pointer says to callers that the callee *will* preserve the frame pointer, no matter which toolchain compiled the callee. If we can't guarantee that, then we need to do something different. > + > + NB: All registers, including disabled registers, aren't preserved > + so that ABI_NO_CALLEE_SAVED can be used for all callers and callees, > + including callers with additional registers enabled by target > + attribute. Disabled registers won't be caller-saved since they > + won't be accessed in the caller after > + > + commit 3050eeeee4183b9723a1465552b5798a1c21d223 > + Author: H.J. Lu <[email protected]> > + Date: Fri May 15 08:01:56 2026 +0800 > + > + caller_save_regs: Return the enabled registers > + > + */ > > static const predefined_function_abi & > ix86_no_callee_saved_abi (void) > { > auto &no_callee_saved_abi = function_abis[ABI_NO_CALLEE_SAVED]; > if (!no_callee_saved_abi.initialized_p ()) > - ix86_initialize_abi (ABI_NO_CALLEE_SAVED, accessible_reg_set); > + { > + HARD_REG_SET full_reg_clobbers = reg_class_contents[ALL_REGS]; > + ix86_initialize_abi (ABI_NO_CALLEE_SAVED, full_reg_clobbers); > + } > return no_callee_saved_abi; > } I'd gone back and forth on whether accessible_reg_set or reg_class_contents[ALL_REGS] was stylistically better. But despite what the comment says, I think the behaviour is the same either way. In particular, the first thing that ix86_initialize_abi does is: /* The general rule is that fixed registers should be marked as call-clobbered. This includes global registers, inaccessible registers, the flags register, and the FPSR. Handle the exceptions below. */ full_reg_clobbers |= fixed_reg_set; and that would set full_reg_clobbers to ALL_REGS even if we started with accessible_reg_set. Remember that this function_abi is specific to the current target, and thus to the current accessible_reg_set. (See target-globals.h and the way that ix86_set_current_function switches targets.) That means that we don't have to worry about additional registers. If additional registers are enabled, we'd switch to a different target, and that target would have its own version of this ABI, with a different accessible_reg_set. That is, accessible_reg_set and function_abis[ABI_NO_CALLEE_SAVED] change in tandem. We don't reuse function_abis[ABI_NO_CALLEE_SAVED] for multiple accessible_reg_sets. I agree that reg_class_contents[ALL_REGS] is probably more consistent with the other functions. But I think the NB is misleading. This is purely a stylistic choice. So my recommendation would be to keep the code change but drop the comment change. Or perhaps we should change the comment to say something else, since I suppose this discussion proves that the situation isn't obvious. :) -- There was a typo in one of my patches, sorry. I've attached a patch below. -- How would you like to handle the submission? Will you submit the x86 prepatches and the main patch as a single series, to show the prepatches in context? Or would you prefer me to send the prepatches separately? Either way's fine with me. Thanks, Richard diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 19bd8b7cbf3..f195a96359e 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -518,7 +518,7 @@ (ABI_DEFAULT 0) ;; An ABI that describes the effect of vzeroupper. vzeroupper is ;; represented as a call because calls are the main mechanism for - ;; expression partial register clobbers. + ;; expressing partial register clobbers. (ABI_VZEROUPPER 1) ;; ABI_ALTERNATE is the Windows function ABI if ix86_abi == SYSV_ABI ;; and is the SYSV function ABI if ix86_abi == MS_ABI.
