View Issue Details

IDProjectCategoryView StatusLast Update
0037139FPCCompilerpublic2020-08-12 18:27
ReporterBi0T1N Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
Product Version3.3.1 
Summary0037139: Add InterlockedAdd compiler intrinsic
DescriptionThe InterlockedAdd intrinsic should perform an atomic addition operation with the specified values.
It works like the InterlockedIncrement/InterlockedDecrement function, just with a second operand - the number instead of using hardcoded 1/-1 in the assembler.

Additionally InterlockedIncrement/InterlockedDecrement could be replaced by InterlockedAdd.
Additional Informationhttps://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html#g_t_005f_005fatomic-Builtins (__atomic_add_fetch )
https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedadd
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Activities

Thaddy de Koning

2020-05-25 09:14

reporter   ~0123055

Last edited: 2020-05-25 09:15

View 2 revisions

This is being worked on. This intrinsic is already there, but we can add an alias.
First I will do the Atomic vs Interlocked aliases. Then will evaluate what is still missing.
This one is already there in several forms. As you can examine in systemh.inc.

Jonas Maebe

2020-05-25 11:38

manager   ~0123057

There is already InterlockedExchangeAdd which does that.

Bi0T1N

2020-05-29 17:39

reporter   ~0123129

Last edited: 2020-05-29 17:39

View 2 revisions

InterlockedExchangeAdd doesn't do exactly the same as it returns the previous value while InterlockedAdd returns the result of the operation (=new value).
Following the GCC naming convention InterlockedExchangeAdd does fetch + add while InterlockedAdd does add + fetch.


function InterlockedAdd(var Target: LongInt; Source: LongInt):LongInt;
var
  OldValue: LongInt;
begin
  OldValue := InterlockedExchangeAdd(Target, Source);
  Result := OldValue + Source;
end;


but calling InterlockedAdd with 1/-1 would be the same as InterlockedIncrement/InterlockedDecrement and thus they are obsolete once implemented.

Jonas Maebe

2020-05-29 21:52

manager   ~0123133

We generally try to be compatible with Delphi code rather than gcc, and I don't see any reason why we would want to break backward compatibility by removing InterlockedIncrement/Decrement. I really don't see the advantage of adding these intrinsics, as it's not like the semantics of InterlockedAdd are better than those of InterlockedExchangeAdd (nor worse, for that matter).

Sven Barth

2020-05-30 11:29

manager   ~0123144

Plus the System.Interlocked* functions are usually implemented in assembly and it might be that a platform can handle Increment/Decrement using a more efficient instruction sequence than an arbitrary Add.

Bi0T1N

2020-05-30 21:39

reporter   ~0123150

Last edited: 2020-05-30 22:13

View 4 revisions

Actually it's needed for Delphi compatibility - they just changed their naming convention and named it AtomicIncrement.
However, I think it could be added in system.inc with the implementation I gave above as it needs less changes than changing the existing implementations of InterlockedIncrement/InterlockedDecrement to accept a second argument.
And I didn't checked all assembly implementations but all I've seen are using hardcoded 1/-1 and that's why I said InterlockedIncrement/InterlockedDecrement could also be replaced with the new function which accepts the value as a second parameter. I didn't said it has to be replaced.

Bi0T1N

2020-06-07 22:50

reporter   ~0123320

Please find attached a patch which adds InterlockedAdd and AtomicIncrement (Delphi compatibility). If only Delphi compatibility is desired I could also provide a patch for that.
01-Add_InterlockedAdd_and_AtomicIncrement.patch (6,392 bytes)   
diff --git rtl/inc/system.inc rtl/inc/system.inc
index f4489ceb98..a9b648c972 100644
--- rtl/inc/system.inc
+++ rtl/inc/system.inc
@@ -869,6 +869,68 @@ function InterLockedCompareExchangePointer (var Target: pointer; NewValue: point
 {$endif FPC_SYSTEM_HAS_EXPLICIT_INTERLOCKED_POINTER}
 {$endif FPC_HAS_EXPLICIT_INTERLOCKED_POINTER}
 
+{$ifdef cpu16}
+function InterlockedAdd (var Target: smallint; Increment: smallint): smallint;
+var
+  OldValue: smallint;
+begin
+  OldValue := InterlockedExchangeAdd(Target, Increment);
+  Result := OldValue + Increment;
+end;
+
+function InterlockedAdd (var Target: word; Increment: word): word;
+var
+  OldValue: word;
+begin
+  OldValue := InterlockedExchangeAdd(Target, Increment);
+  Result := OldValue + Increment;
+end;
+{$endif cpu16}
+
+function InterlockedAdd (var Target: longint; Increment: longint): longint;
+var
+  OldValue: longint;
+begin
+  OldValue := InterlockedExchangeAdd(Target, Increment);
+  Result := OldValue + Increment;
+end;
+
+function AtomicIncrement (var Target: longint; Increment: longint): longint;
+begin
+  Result := InterlockedAdd(Target, Increment);
+end;
+
+function InterlockedAdd (var Target: cardinal; Increment: cardinal): cardinal;
+var
+  OldValue: cardinal;
+begin
+  OldValue := InterlockedExchangeAdd(Target, Increment);
+  Result := OldValue + Increment;
+end;
+
+{$ifdef cpu64}
+function InterlockedAdd (var Target: int64; Increment: int64): int64;
+var
+  OldValue: int64;
+begin
+  OldValue := InterlockedExchangeAdd64(Target, Increment);
+  Result := OldValue + Increment;
+end;
+
+function AtomicIncrement (var Target: int64; Increment: int64): int64;
+begin
+  Result := InterlockedAdd(Target, Increment);
+end;
+
+function InterlockedAdd (var Target: qword; Increment: qword): qword;
+var
+  OldValue: qword;
+begin
+  OldValue := InterlockedExchangeAdd64(Target, Increment);
+  Result := OldValue + Increment;
+end;
+{$endif cpu64}
+
 procedure fpc_objecterror; compilerproc;
 begin
   HandleErrorAddrFrameInd(210,get_pc_addr,get_frame);
diff --git rtl/inc/systemh.inc rtl/inc/systemh.inc
index d455c4517c..6773afd865 100644
--- rtl/inc/systemh.inc
+++ rtl/inc/systemh.inc
@@ -1492,18 +1492,23 @@ function InterlockedDecrement (var Target: smallint) : smallint; public name 'FP
 function InterlockedExchange (var Target: smallint;Source : smallint) : smallint; public name 'FPC_INTERLOCKEDEXCHANGE16';
 function InterlockedExchangeAdd (var Target: smallint;Source : smallint) : smallint; public name 'FPC_INTERLOCKEDEXCHANGEADD16';
 function InterlockedCompareExchange(var Target: smallint; NewValue: smallint; Comperand: smallint): smallint; public name 'FPC_INTERLOCKEDCOMPAREEXCHANGE16';
+function InterlockedAdd (var Target: smallint; Increment: smallint): smallint;
 {$endif cpu16}
 function InterlockedIncrement (var Target: longint) : longint; public name 'FPC_INTERLOCKEDINCREMENT';
 function InterlockedDecrement (var Target: longint) : longint; public name 'FPC_INTERLOCKEDDECREMENT';
 function InterlockedExchange (var Target: longint;Source : longint) : longint; public name 'FPC_INTERLOCKEDEXCHANGE';
 function InterlockedExchangeAdd (var Target: longint;Source : longint) : longint; public name 'FPC_INTERLOCKEDEXCHANGEADD';
 function InterlockedCompareExchange(var Target: longint; NewValue: longint; Comperand: longint): longint; public name 'FPC_INTERLOCKEDCOMPAREEXCHANGE';
+function InterlockedAdd (var Target: longint; Increment: longint): longint;
+function AtomicIncrement (var Target: longint; Increment: longint = 1): longint;
 {$ifdef cpu64}
 function InterlockedIncrement64 (var Target: int64) : int64; public name 'FPC_INTERLOCKEDINCREMENT64';
 function InterlockedDecrement64 (var Target: int64) : int64; public name 'FPC_INTERLOCKEDDECREMENT64';
 function InterlockedExchange64 (var Target: int64;Source : int64) : int64; public name 'FPC_INTERLOCKEDEXCHANGE64';
 function InterlockedExchangeAdd64 (var Target: int64;Source : int64) : int64; public name 'FPC_INTERLOCKEDEXCHANGEADD64';
 function InterlockedCompareExchange64(var Target: int64; NewValue: int64; Comperand: int64): int64; public name 'FPC_INTERLOCKEDCOMPAREEXCHANGE64';
+function InterlockedAdd (var Target: int64; Increment: int64): int64;
+function AtomicIncrement (var Target: int64; Increment: int64 = 1): int64;
 {$endif cpu64}
 { Pointer overloads }
 {$if defined(FPC_HAS_EXPLICIT_INTERLOCKED_POINTER)}
@@ -1539,18 +1544,21 @@ function InterlockedDecrement (var Target: word) : word; external name 'FPC_INTE
 function InterlockedExchange (var Target: word;Source : word) : word; external name 'FPC_INTERLOCKEDEXCHANGE16';
 function InterlockedExchangeAdd (var Target: word;Source : word) : word; external name 'FPC_INTERLOCKEDEXCHANGEADD16';
 function InterlockedCompareExchange(var Target: word; NewValue: word; Comperand: word): word; external name 'FPC_INTERLOCKEDCOMPAREEXCHANGE16';
+function InterlockedAdd (var Target: word; Increment: word): word;
 {$endif cpu16}
 function InterlockedIncrement (var Target: cardinal) : cardinal; external name 'FPC_INTERLOCKEDINCREMENT';
 function InterlockedDecrement (var Target: cardinal) : cardinal; external name 'FPC_INTERLOCKEDDECREMENT';
 function InterlockedExchange (var Target: cardinal;Source : cardinal) : cardinal; external name 'FPC_INTERLOCKEDEXCHANGE';
 function InterlockedExchangeAdd (var Target: cardinal;Source : cardinal) : cardinal; external name 'FPC_INTERLOCKEDEXCHANGEADD';
 function InterlockedCompareExchange(var Target: cardinal; NewValue: cardinal; Comperand: cardinal): cardinal; external name 'FPC_INTERLOCKEDCOMPAREEXCHANGE';
+function InterlockedAdd (var Target: cardinal; Increment: cardinal): cardinal;
 {$ifdef cpu64}
 function InterlockedIncrement64 (var Target: qword) : qword; external name 'FPC_INTERLOCKEDINCREMENT64';
 function InterlockedDecrement64 (var Target: qword) : qword; external name 'FPC_INTERLOCKEDDECREMENT64';
 function InterlockedExchange64 (var Target: qword;Source : qword) : qword; external name 'FPC_INTERLOCKEDEXCHANGE64';
 function InterlockedExchangeAdd64 (var Target: qword;Source : qword) : qword; external name 'FPC_INTERLOCKEDEXCHANGEADD64';
 function InterlockedCompareExchange64(var Target: qword; NewValue: qword; Comperand: qword): int64; external name 'FPC_INTERLOCKEDCOMPAREEXCHANGE64';
+function InterlockedAdd (var Target: qword; Increment: qword): qword;
 {$endif cpu64}
 
 

Thaddy de Koning

2020-07-12 10:51

reporter   ~0123915

Last edited: 2020-07-12 12:04

View 9 revisions

The patch is not correct. It is sufficient to add the following code at the correct place in system.
Since I am not totaly sure where? I have a test unit that works already separately on most platforms, but I leave it to devel's to copy just the declarations to the right place in systemh.inc:

----- This should go in system, in the right place ----
unit atomunit;

interface
{ delphi compatibility aliases, this should go in systemh.inc }
function AtomicIncrement (var Target: longint) : longint; external name 'FPC_INTERLOCKEDINCREMENT';
function AtomicDecrement (var Target: longint) : longint; external name 'FPC_INTERLOCKEDDECREMENT';
function AtomicExchange (var Target: longint;Source : longint) : longint; external name 'FPC_INTERLOCKEDEXCHANGE';
function AtomicExchangeAdd (var Target: longint;Source : longint) : longint; external name 'FPC_INTERLOCKEDEXCHANGEADD';
function AtomicCmpExchange(var Target: longint; NewValue: longint; Comperand: longint): longint; external name 'FPC_INTERLOCKEDCOMPAREEXCHANGE';

implementation
// none, is already done
end.

This is exactly how some of the others are implemented, the code is already there.
Note I actually DID patch my system unit to test this and here it works, but seeing the code I am not sure to provide this as a patch -platforms -, so that's why I am submitting a test unit, but the code should be in the interface section of system.
For one of the devel's - or the submitter - this should be very easy to fix based on the above..

Bi0T1N

2020-07-12 20:46

reporter   ~0123939

@Thaddy de Koning
No, your approach isn't correct. It's not just about adding the aliases, it's also about supporting the second parameter.


function AtomicIncrement(var Target; [Increment]): Integer; overload;
function AtomicIncrement(var Target; [Increment]): Int64; overload;


http://docwiki.embarcadero.com/Libraries/Rio/en/System.AtomicIncrement

Thaddy de Koning

2020-07-12 21:48

reporter   ~0123945

It is more correct than your solution.
I am sure the wise men will combine it......
(one thing that bothers me is that you did not even look at the code...)

Thaddy de Koning

2020-07-13 14:54

reporter   ~0123981

compared to D10.3 my solution is complete and even one bonus.

Thaddy de Koning

2020-07-14 10:29

reporter   ~0123997

Last edited: 2020-07-14 10:41

View 3 revisions

Here's some more:
function AtomicIncrement64 (var Target: int64) : int64; external name 'FPC_INTERLOCKEDINCREMENT64';
function AtomicDecrement64 (var Target: int64) : int64; external name 'FPC_INTERLOCKEDDECREMENT64';
function AtomicExchange64 (var Target: int64;Source : int64) : int64; external name 'FPC_INTERLOCKEDEXCHANGE64';
function AtomicExchangeAdd64 (var Target: int64;Source : int64) : int64; external name 'FPC_INTERLOCKEDEXCHANGEADD64';
function AtomicCmpExchange64(var Target: int64; NewValue: int64; Comperand: int64): int64; external name 'FPC_INTERLOCKEDCOMPAREEXCHANGE64';

These should ideally be overloads w/o the 64 to keep Delphi compatibility, Delphi seems to support this only for D64, not for D32

Thaddy de Koning

2020-07-14 14:57

reporter   ~0124008

Last edited: 2020-07-14 15:00

View 3 revisions

If you mean the [increment] part: that is not a parameter, but a compiler hint and FPC does not need it.
The 10 declarations I provided cover all 8 as declared in Delphi plus two more. (apart from the 64 addition, but I now have overloads.)
I can provide a patch for 32 and 64 bit. About 16 bit I am not sure and that can cause more trouble than good. (and Delphi does no longer support 16 bit anyway)

It really is only a interface section part: everything and more is already implemented.

Sven Barth

2020-07-14 16:13

manager   ~0124010

@Thaddy: the [Increment] part is a parameter. Delphi does not have separate AtomicAdd functions instead its part of AtomicIncrement and AtomicDecrement.

Thaddy de Koning

2020-07-14 18:34

reporter   ~0124012

Last edited: 2020-07-14 18:39

View 2 revisions

So I am right that with the overloads that can be ignored?
Because in effect my code behaves the same to the user.

Sven Barth

2020-08-11 23:06

manager   ~0124780

@Thaddy: No, they can not be ignored. "AtomicIncrement(SomeVar, 4)" is atomically adding 4 to "SomeVar".

@Bi0T1N: I've tested a bit with Delphi. They support essentially any integer type from Int8 to Int64 and UInt8 to UInt64 including range types and Native(U)Int. As all these can't be handled by an overload we will probably have to go the way of real intrinsics (an intrinsic can simply check for orddef that isn't an enumdef (or void)).

Bi0T1N

2020-08-12 14:24

reporter   ~0124796

Last edited: 2020-08-12 14:31

View 2 revisions

So to summarize:
- we'd need InterlockedExchangeAdd to support Byte and Shortint -> overload to call the version of Longint/Longword?
- the {$ifdef cpu16} for InterlockedCompareExchange needs to be removed to support Smallint and Word everywhere + overload to call the version of Longint/Longword on non-cpu16?
- all other types are already supported (does 32-bit Delphi supports Int64/QWord? If yes, {$ifdef cpu64} also needs to be removed)

If I got you right the AtomicIncrement intrinsic should then create the call to the appropriate InterlockedCompareExchange function? (As the ROL/ROR/SAR intrinsics do?)
Could you please also upload your tests - or didn't you created a real test suite?

Sven Barth

2020-08-12 16:44

manager   ~0124802

We need all Interlocked* functions with support for 8 - 64 bit (yes, 32-bit supports it for 64-bit values as well). The default implementation of the intrinsic will call the Interlocked* functions, but they can be replaced depending on the CPU if something more effective is available (this would also allow to get rid of calls)

And no, I've not made a testsuite, I've simply played around with Delphi a bit. I'll see if I can cook something up that we can use both in Delphi and for FPC.

Issue History

Date Modified Username Field Change
2020-05-25 01:36 Bi0T1N New Issue
2020-05-25 09:14 Thaddy de Koning Note Added: 0123055
2020-05-25 09:15 Thaddy de Koning Note Edited: 0123055 View Revisions
2020-05-25 11:38 Jonas Maebe Note Added: 0123057
2020-05-29 17:39 Bi0T1N Note Added: 0123129
2020-05-29 17:39 Bi0T1N Note Edited: 0123129 View Revisions
2020-05-29 21:52 Jonas Maebe Note Added: 0123133
2020-05-30 11:29 Sven Barth Note Added: 0123144
2020-05-30 21:39 Bi0T1N Note Added: 0123150
2020-05-30 22:06 Bi0T1N Note Edited: 0123150 View Revisions
2020-05-30 22:10 Bi0T1N Note Edited: 0123150 View Revisions
2020-05-30 22:13 Bi0T1N Note Edited: 0123150 View Revisions
2020-06-07 22:50 Bi0T1N Note Added: 0123320
2020-06-07 22:50 Bi0T1N File Added: 01-Add_InterlockedAdd_and_AtomicIncrement.patch
2020-07-12 10:51 Thaddy de Koning Note Added: 0123915
2020-07-12 10:54 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 10:58 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 10:59 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 11:05 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 11:45 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 11:52 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 11:57 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 12:04 Thaddy de Koning Note Edited: 0123915 View Revisions
2020-07-12 20:46 Bi0T1N Note Added: 0123939
2020-07-12 21:48 Thaddy de Koning Note Added: 0123945
2020-07-13 14:54 Thaddy de Koning Note Added: 0123981
2020-07-14 10:29 Thaddy de Koning Note Added: 0123997
2020-07-14 10:32 Thaddy de Koning Note Edited: 0123997 View Revisions
2020-07-14 10:41 Thaddy de Koning Note Edited: 0123997 View Revisions
2020-07-14 14:57 Thaddy de Koning Note Added: 0124008
2020-07-14 14:58 Thaddy de Koning Note Edited: 0124008 View Revisions
2020-07-14 15:00 Thaddy de Koning Note Edited: 0124008 View Revisions
2020-07-14 16:13 Sven Barth Note Added: 0124010
2020-07-14 18:34 Thaddy de Koning Note Added: 0124012
2020-07-14 18:39 Thaddy de Koning Note Edited: 0124012 View Revisions
2020-08-11 23:06 Sven Barth Note Added: 0124780
2020-08-12 14:24 Bi0T1N Note Added: 0124796
2020-08-12 14:31 Bi0T1N Note Edited: 0124796 View Revisions
2020-08-12 16:44 Sven Barth Note Added: 0124802