On Tue, Sep 02, 2025 at 12:12:43PM +0200, Jakub Jelinek wrote: > On Tue, Sep 02, 2025 at 05:01:37AM -0500, Segher Boessenkool wrote: > > On Tue, Sep 02, 2025 at 07:56:58AM +0200, Jakub Jelinek wrote: > > > On Tue, Sep 02, 2025 at 09:32:15AM +0530, Surya Kumari Jangala wrote: > > > > Ping. > > > > > > > > Please review. > > > > > > IMHO we shouldn't introduce new builtins for this, but instead > > > use new flag bits in the upper bits of the memorder arguments. > > > See how x86 uses __ATOMIC_HLE_ACQUIRE and __ATOMIC_HLE_RELEASE > > > in there. x86-Specific Memory Model Extensions for Transactional Memory > > > in the documentation provides an example. > > > > This doesn't change the memorder (or anything else): a load-locked with > > EH=1 is semantically equivalent to a load-locked with EH=0. It just > > behaves differently in the microarchitecture so there is a different > > performance profile with it. EH=1 tells the implementation to optimise > > for the case that no other agent is involved at all: > > > > EH = 1 should be used when the program is obtaining > > a lock variable which it will subsequently release > > before another program attempts to perform a store > > to it. When contention for a lock is significant, > > using this hint may reduce the number of times a > > cache block is transferred between processor caches. > > > > Hiding this in an unrelated parameter is obfuscation. I'd rather not. > > It is not unrelated, the upper bits of the memmodel enum are reserved for > target flags and that is exactly what x86 was implementing for the HLE > variants.
Huh. What a terrible design, in itself and it does not mesh at all with the rest of RTL design! > So it follows the existing design, which adding arbitrary suffixes to > existing builtins doesn't. > See also memmodel.h header. It is exactly for cases like this, > it is an atomic compare and exchange with some target specific details. Having random magical values in random parameters is not something that belongs in a generic API, nor in a target-specific one for that matter, but certainly not in a generic one. The proposal is to **not** have it be target-specific! We can do a target-specific thing just fine (and it will not abuse bits in an unrelated parameter then either, heh, we do have good taste). But we thought it would be a useful thing for other targets to be able to treat "local" changes (data that stays on a "home node)" specially. If people disagree, then fine, we'll constrain the benefits to rs6000 only :-) > > (EH=1 is a variant of the larx insn, the LL in LL/SC. This patch is to > > make a __atomic_compare_exchange_local which has the same semantics as > > __atomic_compare_exchange, just perhaps different performance). We'll need to do a separate builtin for that much more fundamental operation anyway. To have a compare<->exchange built on that makes sense (for us at least), it is the main case where you want the EH=1 treatment anyway. And a nice generic (but pretty vague) name for it is useful IMO. Segher
