Re: [OG12][committed] amdgcn, libgomp: custom USM allocator

[Andrew Stubbs <ams@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

I changed it to use 128-byte alignment to match the GPU cache-lines.

Committed to OG12.

Andrew

On 11/01/2023 18:05, Andrew Stubbs wrote:
> This patch fixes a runtime issue I encountered with the AMD GCN Unified 
> Shared Memory implementation.
> 
> We were using regular malloc'd memory configured into USM mode, but 
> there were random intermittent crashes. I can't be completely sure, but 
> my best guess is that the HSA driver is using malloc internally from the 
> same heap, and therefore using memory on the same page as the offload 
> kernel. What I do know is that I could make the crashes go away by 
> simply padding the USM allocations before and after.
> 
> With this patch USM allocations are now completely separated from the 
> system heap. The custom allocator is probably less optimal is some 
> use-cases, but does have the advantage that all the metadata is stored 
> in a side-table that won't ever cause any pages to migrate back to 
> main-memory unnecessarily. It's still possible for the user program to 
> use USM memory in a way that causes it to thrash, and this might have 
> been the ultimate cause of the crashes, but there's not much we can do 
> about that here.
> 
> I've broken the allocator out into a new file because I anticipate it 
> being needed in more than one place, but I didn't put full 
> data-isolation on it yet.
> 
> I'll rebase, merge, and repost all of the OpenMP memory patches sometime 
> soonish.
> 
> Andrew

[-- Attachment #2: 230113-usm-align-128.patch --]
[-- Type: text/plain, Size: 839 bytes --]

libgomp, amdgcn: Switch USM to 128-byte alignment

This should optimize cache-lines on the AMD GPUs somewhat.

libgomp/ChangeLog:

	* usm-allocator.c (ALIGN): Use 128-byte alignment.

diff --git a/libgomp/usm-allocator.c b/libgomp/usm-allocator.c
index c45109169ca..68c1ebafec2 100644
--- a/libgomp/usm-allocator.c
+++ b/libgomp/usm-allocator.c
@@ -57,7 +57,8 @@ static int usm_lock = 0;
 static struct usm_splay_tree_s usm_allocations = { NULL };
 static struct usm_splay_tree_s usm_free_space = { NULL };
 
-#define ALIGN(VAR) (((VAR) + 7) & ~7)    /* 8-byte granularity.  */
+/* 128-byte granularity means GPU cache-line aligned.  */
+#define ALIGN(VAR) (((VAR) + 127) & ~127)
 
 /* Coalesce contiguous free space into one entry.  This considers the entries
    either side of the root node only, so it should be called each time a new