[Bug 54832] New: Dereference with non-constant offset of type HLSL_IR_EXPR
https://bugs.winehq.org/show_bug.cgi?id=54832 Bug ID: 54832 Summary: Dereference with non-constant offset of type HLSL_IR_EXPR Product: vkd3d Version: 1.7 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: hlsl Assignee: wine-bugs(a)winehq.org Reporter: flibitijibibo(a)gmail.com Distribution: --- Created attachment 74335 --> https://bugs.winehq.org/attachment.cgi?id=74335 EG2 Vertex Shader MojoShader uses this for arrays, as an example: --- float4 uniforms_float4[13]; int4 m_v6 : BLENDINDICES0; const int ARRAYBASE_8 = 0; a0.x = int(floor(abs(v6.x) + 0.5) * sign(v6.x)); r0.xy = lerp(v1.xy, v1.zw, uniforms_float4[ARRAYBASE_8 + a0.x].xy); --- A complete example from Escape Goat 2 is attached, but this shader depends on #54826 getting fixed first. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #1 from Ethan Lee <flibitijibibo(a)gmail.com> --- I've uploaded a beta branch of Escape Goat 2 on Steam for Linux that reproduces this issue. The password is "codeweaversrulez". This is the same game as the default build, but it includes dxvk-native and vkd3d in the ./lib64/ folder, so launching the game with "/gldevice:D3D11" as a launch option should be enough to get it to run (and immediately crash). The debug logs will be at /tmp/EscapeGoat_DebugLog.txt, and you should be able to spot the compiler error messages toward the bottom. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #2 from Ethan Lee <flibitijibibo(a)gmail.com> --- Played with this a bit today... long story short, the hlsl_fixme is exactly where it should be (big surprise), but I'm definitely out of my league here as I'm not totally sure what a fix for this would entail. I can see how everything works for IR_CONSTANT offsets, but IR_EXPR and IR_LOAD (or maybe IR_INDEX?) seem to complicate things a _ton_ because without knowing the offset ahead of time, it can't figure out where to load or store anything from, and it doesn't seem like there's an easy way to defer that work to the shader itself. The asm isn't too helpful either: HLSL: const int ARRAYBASE_8 = 0; r0.xy = lerp(v1.xy, v1.zw, uniforms_float4[ARRAYBASE_8 + a0.x].xy); SM4 asm: add r0.yzw, -v1.yyxy, v1.wwzw mad r1.xy, cb0[r0.x + 0].xyxx, r0.zwzz, v1.xyxx Like I said though, this is definitely uncharted waters for me, so maybe I'm missing something obvious here. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #3 from Ethan Lee <flibitijibibo(a)gmail.com> --- Submitted a unit test, still with no fix, but should at least be useful for someone that's smarter than me! https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/189 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #4 from Ethan Lee <flibitijibibo(a)gmail.com> --- Created attachment 74416 --> https://bugs.winehq.org/attachment.cgi?id=74416 Awful non-const patch that sucks Attached is a very very bad diff that gets non-const offsets to compile. It fails a bunch of "unrelated" unit tests and is riddled with FIXMEs! It occurred to me that the HLSL compiler might not support this, but the DXBC parser still could - and it does. So I turned this from a compiler task into more of a reverse-engineering task, and this patch is the result. The core of this feature lives in the TPF emitter; IR_EXPR needs to take a separate path from IR_CONSTANT, doing a number of _very_ specific things: 1. The register being returned by sm4_register_from_deref should something other than TEMP (in our case, CONSTBUFFER) 2. The index, rather than a literal, is a temp register, which is handed to us via deref->offset.node->reg 3. The index information is shoved at the end of the src info; it's basically a nested src info inside a src info 4. With the nested src info, we need to mark the base src as having an additional order with relative addressing enabled; this also means we have to add the nested src length to the total size of the instruction The big issues with this patch: - I couldn't figure out where some of the data comes from - a lot of these seems like it should be provided by the hlsl_ir_expr, and I'm sure it's in there _somewhere_, but I wasn't able to find it myself - The change to existing tokens is a mess - grep "has_deref" and try not to bleed too much afterward - The added fields to sm4_register seems to have introduced some uninitialized memory issues, which is what I'm guessing is causing all the broken tests (has_deref, as ugly as it is, is there to try and isolate the ugliness to this one specific bit of input data) I got a whole lot further than I expected, but I still think this will need a maintainer's touch to get all the details right. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #5 from Ethan Lee <flibitijibibo(a)gmail.com> --- First MR that will eventually fix this: https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/207 Currently testing with the full patch series and it does appear to work with my tests - will report back if anything comes up but it looks like this will be resolved when the final patch is approved. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #6 from Ethan Lee <flibitijibibo(a)gmail.com> --- A unit test has been upstreamed and can be found here: https://gitlab.winehq.org/wine/vkd3d/-/blob/master/tests/array-index-expr.sh... More tests will be added with the current MR as well. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #7 from Ethan Lee <flibitijibibo(a)gmail.com> --- The following commit fixes this error for vectors: https://gitlab.winehq.org/wine/vkd3d/-/commit/ebf7573571d4bfcd3f38846886106f... Once the patch for SM4 relative addressing is merged this should be fully resolved for arrays as well. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #8 from Ethan Lee <flibitijibibo(a)gmail.com> --- This MR should fully resolve this issue: https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/229 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #9 from Ethan Lee <flibitijibibo(a)gmail.com> --- Previous MR has been replaced with a new series, starting with this part: https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/396 I've tested the full series against some FNA titles and my MojoShader database and all came out clean! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #10 from Ethan Lee <flibitijibibo(a)gmail.com> --- MR that resolves this issue take 2: https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/435 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 Ethan Lee <flibitijibibo(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Fixed by SHA1| |0ef25ad137d076490b87edabfd5 | |73546ae9b8ff4 Resolution|--- |FIXED --- Comment #11 from Ethan Lee <flibitijibibo(a)gmail.com> --- Fixed in the latest Git revision! -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #12 from Nikolay Sivov <bunglehead(a)gmail.com> --- Closing bugs fixed in 1.10. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=54832 --- Comment #13 from Andrey Gusev <andrey.goosev(a)gmail.com> --- Created attachment 76484 --> https://bugs.winehq.org/attachment.cgi?id=76484 error Weird, I still get this error with 1.11-556-g9c83caed -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla