On Tue, Sep 2, 2025 at 7:26 PM Segher Boessenkool <[email protected]> wrote: > > 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 :-)
I agree with Jakub here. Richard. > > > (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
