chipStar on rusticl

[pjaaskel]

Moving the discussion from PoCL PR pocl/pocl#1441 here.

[karolherbst]

so afaik the missing features on rusticl's end are program scope variables and generic pointers, the former are a real issue, but for generic pointers I can probably hack up a patch if people are hitting certain asserts (nir_deref_mode_is_in_set: Assertion nir_deref_mode_must_be(deref, modes)' failed.) in testing way too often.

[pjaaskel]

Wasn't the plan for PSV have the "nearly complete" solution that should cover 99.9% of cases available behind an environment variable? It should cover all of chipStar tests and hecbench cases pretty surely (I don't think there are globals with initializers taking an address).

[karolherbst]

yeah... but I also have an idea on how to implement those properly fully, which will also help with SVM later. But yeah, I forgot that I suggested I could implement parts of it behind a flag. I should probably land this as it's not even touching that much code I think...

[linehill]

@karolherbst, is the following rusticl-nouveau double free a known issue? The issue appears on Ubuntu 22.04, linux kernel 6.5.0 (I recall) and with locally built, very recent drm.

[karolherbst]

first time I saw that one... anything specific you've done to trigger it?

[karolherbst]

but there is also an obvious bug in the code, so I'm gonna fix that one anyway,

[karolherbst]

Mind checking if this MR fixes the double-free for you? I'm not quite sure how one would run into that code path, but I'd assume it might be either a very old kernel or the nvidia driver being used? If it's the later it's concerning that we are even getting this far and might want to safe guard against loading nouveau on top of the nvidia driver better. If it's not fixed by that, I'll have to dig more, but that's the only obvious thing I could see in the relevant code here.

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29000

[linehill]

first time I saw that one... anything specific you've done to trigger it?

No, I think - just running clinfo with LD_LIBRARY_PATH pointing to library directories of the local mesa install directory and OCL_ICD_VENDORS pointing to to the location of rusticl.icd.

Mind checking if this MR fixes the double-free for you?

I'll give it a shot.

[karolherbst]

first time I saw that one... anything specific you've done to trigger it?

No, I think - just running clinfo with LD_LIBRARY_PATH pointing to library directories of the local mesa install directory and OCL_ICD_VENDORS pointing to to the location of rusticl.icd.

you can also just do meson devenv from within the build directory and it will set all relevant env variables on its own. But anyway, are you running with nouveau as the kernel driver or nvidia?

[linehill]

But anyway, are you running with nouveau as the kernel driver or nvidia?

Is this related to configuration of the /etc/X11/xorg.conf file? It's something I was not aware of.

[karolherbst]

no, rather which kernel driver you are using for your Nvidia GPU. I'm still confused on how you could hit this code path, so my current explanation is simply that you were trying to use mesa while using the official nvidia driver.

[linehill]

Oh, I see. Definitely I have been using an official nvidia driver.

[linehill]

Couldn’t boot with nouveau using Ubuntu’s stock xserver-xorg-video-nouveau and nouveau-firmware package. The GUI won’t start and I may drop into CLI login for a few seconds before the machine freezes.

[karolherbst]

looks like some firmware issue, which should be fixed with GSP, which requires 6.7 and booting with nouveau.config=NvGspRm=1 on your GPU (it's enabled by default on 40 Series GPUs). Alternatively we might have to ask Nvidia to update the relevant Firmware, but last time it took months.

But anyway, you don't really have to use nouveau on the kernel side, I'm more interested if my patch fixes the crash with your initial config anyway. And you can test the bda support also with the llvmpipe which runs on the CPU.

[linehill]

..., I'm more interested if my patch fixes the crash with your initial config anyway.

It fixes the crash. However, nouveau does not seem to recognize my RTX 3060 (clinfo reports 0 devices) but zink driver does.

[karolherbst]

thanks for confirming!

And yeah, zink will use either vulkan driver, which can be the nvidia one as well. The nouveau mesa driver will only work if you use the nouveau kernel driver as well.

For nouveau specifically it's recommended to use zink anyway, as the compiler used by nouveau mesa driver isn't really ready for compute and using nvk or nvidia through zink will give you better results.

I haven't tested the ext_bda extensions on top of nvidia, but I'd assume it also just works with the latest MR.

[linehill]

A bunch of HeCBench cases segfault at SPIR-V compilation step. I looked into a few of them and they all seem to be triggered by OpenCL built-in calls. I attached a reduced reproducer and a backtrace I got with the reproducer.

ocl-builtin-segfault.tar.gz

[karolherbst]

Seems like this only triggers when you have a function without a name inside the spirv, which we apparently haven't encountered before.

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index f25f67428bd..29c5d78a9df 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -4332,7 +4332,7 @@ static inline nir_function *
 nir_shader_get_function_for_name(const nir_shader *shader, const char *name)
 {
    nir_foreach_function(func, shader) {
-      if (strcmp(func->name, name) == 0)
+      if (func->name && strcmp(func->name, name) == 0)
          return func;
    }
 

[karolherbst]

opened an MR against mesa: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29063

[linehill]

I'll cherry-pick the fix and test it.

[linehill]

The MR fixed the issue.

[linehill]

I’m experiencing a stack overflow / OoM issue in rusticl which seems similar to one fixed on the chipStar in #837. I attached an OpenCL reproducer that segfaults at program exit due to stack overflow caused by >130000 deep call stack (mileage may vary, you may need to increase the LaunchLimit value to trigger the issue). The backtrace of the segfault for the first 200 calls is in rustcl-gdb.log.

rusticl-stack-overflow.tar.gz

[karolherbst]

yeah.. with my fix it stays at ~175MB here, it wasn't an OOM anyway, just a stack overflow which can happen for any reason really, if you trigger a bad pattern like this. And I'm not against fixing it anyway, just don't want the code to become a mess for doing so, for issues like this.

And there is only so much we can do to prevent issues like that in the first place. Like if an app is doing something like this specifically, it's better to use an in-order queue and just not use dependencies like this as it's already guaranteed to be processed in order.

[linehill]

Like if an app is doing something like this specifically, it's better to use an in-order queue and just not use dependencies like this as it's already guaranteed to be processed in order.

It's a minimal reproducer derived from chipStar’s OpenCL backend. chipStar’s OpenCL backend (and the reproducer) is already using in-order queues but we need events for cross-queue synchronization in order to model implicit synchronization behavior of HIP/CUDA streams. We probably could avoid forming the event dependencies in some cases but I’m not sure if we could avoid them completely.

[karolherbst]

yeah... it's a tough problem. And I think the fix I merged might introduce bugs if some of the deps failed, but were released too early, so the the "did all deps ran successfully" part might be wrong in those cases.

I should probably check how other implementations are getting around this issue, because ref counted smart pointer chains causing a stack overflow calling the destructors of its fields sounds like a common issue, though maybe the C++ compilers are just smarter here than doing something potentially recursive.

[karolherbst]

apparently C++ shared_ptr behaves the same here, so other CL implementations might just be smarter about it.

[karolherbst]

fixed it properly now: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29190

But looks like other projects generally do their ref-counting manually and drop the ref to dependencies earlier while still keeping the pointers around. But what I have should be good enough. I think it can still lead to increasing memory usage over time, but uhh, it's a couple of bytes per event and should practically not matter much. There are more pressing issues and performance gains to take care of.

[linehill]

FYI, I ran HeCBench’s HIP benchmarks from this branch on the same device (RTX 3060) through CUDA 12.4 and chipstar->rusticl->zink. Here are speed comparisons of the both:

|Benchmark|Speedup
|--|--
|aidw|0.47
|all|0.23
|bilateral|0.42
|boxfilter|0.59
|bitonic|1.28
|chemv|0.41
|compute|0.43
|entropy|0.76
|fft|0.32
|floydwarshall|0.40
|gaussian|0.54
|haccmk|0.43
|hausdorff|0.57
|histogram|0.37
|hwt1d|0.31
|is|0.88
|jenkins|0.82
|layout|0.46
|lfib4|0.26
|lombscargle|0.15
|mandelbrot|0.13
|maxpool3d|0.83
|minisweep|516.40
|minkowski|0.35
|mrc|0.11
|murmurhash3|0.73
|nw|0.35
|overlay|0.16
|pad|0.23
|perplexity|0.48
|pnpoly|0.69
|present|0.10
|romberg|0.52
|scan2|0.59
|softmax|1.00
|stencil1d|0.47
|swish|0.61
|tqs|0.52
|tsa|0.15
|vanGenuchten|1.43
|--|--
|Geomean|0.49776356626703405

Number = chipstar->rusticl->zink is X times faster than CUDA. I'm suspecting that there is an error in the minisweep benchmark.

[karolherbst]

yeah, minisweep looks out of place. But thanks for the numbers. I should really start optimizing rusticl for perf, because I was mostly focusing on correctness. Anything I'd have to consider compiling those benchmarks again chipstar?

[linehill]

Compiling the benchmarks with debug symbols (make DEBUG=yes or make EXTRA_CFLAGS=-g) may result in JIT compilation failure at runtime due to invalid debug info code in the SPIR-V. I have locally disabled debug info emission for the device code (SPIR-V) by reverting this commit so I have debug info for the host code if needed.

Some benchmarks may (still) need RUSTICL_DEBUG=allow_invalid_spirv to run successfully.

[karolherbst]

I wonder if needing RUSTICL_DEBUG=allow_invalid_spirv explains the minisweep outlier and the compiler doing something unexpected. It might make sense to run those with asserts enabled to see if it would trigger any of them.

[karolherbst]

fyi, I'm also working on an optimization which might impact all of those benchmarks, especially the smaller ones. What I'm working on is to optimize kernels launched with an offset of 0 and a grid size fitting in hw execution (which isn't guaranteed at all), which is like 99.9% of the launches out there. So it would make get_local_id (and all the others) cheaper to execute and I'm curious on how much of a difference that one would make.

[pjaaskel]

I've done a similar optimization in PoCL. PoCL specializes WG launches per goffset=0, local-size, grid size < 64K to make the address computation simpler. It benefits targets with narrow address arithmetics, but also might benefit WG vectorization for the same reason. When I did this for the target, it benefitted a lot as otherwise all addr. computation went to a 64b int datapath which can be expensive in some targets.