Bug 34006 - [glsl regression] multiple games under wine trigger infinite loop in glsl optimization
[glsl regression] multiple games under wine trigger infinite loop in glsl opt...
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: glsl-compiler
git
Other All
: medium normal
Assigned To: Eric Anholt
http://www.regnumonline.com.ar/
:
: 38501 (view as bug list)
Depends on:
Blocks: 41152
  Show dependency treegraph
 
Reported: 2011-02-07 13:14 UTC by Sven Arvidsson
Modified: 2012-01-09 13:19 UTC (History)
3 users (show)

See Also:


Attachments
backtrace of hang (3.37 KB, text/plain)
2011-02-07 13:14 UTC, Sven Arvidsson
Details
a shader triggering the infinite loop (390 bytes, application/octet-stream)
2011-11-28 20:00 UTC, Andy Clayton
Details
IR states that do_swizzle_swizzle flips between (2.09 KB, text/plain)
2011-12-15 21:12 UTC, Andy Clayton
Details
potential patch (1.66 KB, patch)
2012-01-03 21:35 UTC, Andy Clayton
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sven Arvidsson 2011-02-07 13:14:00 UTC
Created attachment 43046 [details]
backtrace of hang

The game Regnum Online hangs on start with git master. Bisecting leads to this:

e31266ed3e3667c043bc5ad1abd65cfdb0fa7fdb is the first bad commit
commit e31266ed3e3667c043bc5ad1abd65cfdb0fa7fdb
Author: Eric Anholt <eric@anholt.net>
Date:   Tue Jan 25 10:28:13 2011 +1000

    glsl: Add a new opt_copy_propagation variant that does it channel-wise.
    
    This patch cleans up many of the extra copies in GLSL IR introduced by
    i965's scalarizing passes.  It doesn't result in a statistically
    significant performance difference on nexuiz high settings (n=3) or my
    demo (n=10), due to brw_fs.cpp's register coalescing covering most of
    those extra moves anyway.  However, it does make the debug of wine's
    GLSL shaders much more tractable, and reduces instruction count of
    glsl-fs-convolution-2 from 376 to 288.

:040000 040000 432bc89bd981ceeffe9a0909f4014c2f0e96db9b f7b00c9109fae73e681f0cb5c7bbecb4c361408e M	src
Comment 1 Tobias Jakobi 2011-02-08 04:49:29 UTC
This also produces a crash in wine (using FEAR demo) on r600g. Commenting out the opt_copy_propagation_elements pass solves the problem and the crash is gone.

Stack trace from the crash:
 0 0x7d8f9c6d _ZN36ir_copy_propagation_elements_visitor4killEP10kill_entry+0x7d() in r600_dri.so (0x00f9e454)
 1 0x7d8fa4fa _ZN36ir_copy_propagation_elements_visitor11visit_leaveEP13ir_assignment+0xc9() in r600_dri.so (0x00f9e484)
 2 0x7d9b8d32 _ZN13ir_assignment6acceptEP23ir_hierarchical_visitor+0xb1() in r600_dri.so (0x00f9e4b4)
 3 0x7d9b8715 visit_list_elements+0x34(v=0xf9e5b4, l=0x6d188130) [/root/git-mesa/src/glsl/ir_hv_accept.cpp:48] in r600_dri.so (0x00f9e4f4)
 4 0x7d8f9f85 _ZN36ir_copy_propagation_elements_visitor11visit_enterEP21ir_function_signature+0x94() in r600_dri.so (0x00f9e514)
 5 0x7d9b884e _ZN21ir_function_signature6acceptEP23ir_hierarchical_visitor+0x1d() in r600_dri.so (0x00f9e544)
 6 0x7d9b8715 visit_list_elements+0x34(v=0xf9e5b4, l=0x6d1880e0) [/root/git-mesa/src/glsl/ir_hv_accept.cpp:48] in r600_dri.so (0x00f9e564)
 7 0x7d9b88ff _ZN11ir_function6acceptEP23ir_hierarchical_visitor+0x4e() in r600_dri.so (0x00f9e594)
 8 0x7d9b8715 visit_list_elements+0x34(v=0xf9e5b4, l=0x6d16c7b8) [/root/git-mesa/src/glsl/ir_hv_accept.cpp:48] in r600_dri.so (0x00f9e5f4)
 9 0x7d8fa3d0 do_copy_propagation_elements+0x9f(instructions=0x6d16c7b8) [/root/git-mesa/src/glsl/opt_copy_propagation_elements.cpp:455] in r600_dri.so (0x00f9e644)
10 0x7d8ee646 do_common_optimization+0x85(ir=0x6d16c7b8, linked=true, max_unroll_iterations=0x20) [/root/git-mesa/src/glsl/glsl_parser_extras.cpp:767] in r600_dri.so (0x00f9e694)
11 0x7d9c26ef link_shaders+0x26e(ctx=0x7d6ed0f0, prog=0x6d31d2c8) [/root/git-mesa/src/glsl/linker.cpp:1630] in r600_dri.so (0x00f9e6d4)
12 0x7d98e96f _mesa_glsl_link_shader+0x20e(ctx=0x7d6ed0f0, prog=0x6d31d2c8) [/root/git-mesa/src/mesa/program/ir_to_mesa.cpp:3211] in r600_dri.so (0x00f9e834)
13 0x7d94bb39 link_program+0x78() in r600_dri.so (0x00f9e8c4)
14 0x7ea843b0 in wined3d (+0x843af) (0x00f9e8f4)
15 0x7ea28a95 in wined3d (+0x28a94) (0x00f9e954)
16 0x7ea64a65 in wined3d (+0x64a64) (0x00f9ebd4)
17 0x7ea39331 in wined3d (+0x39330) (0x00f9ec24)
18 0x7eb38e86 in d3d9 (+0x8e85) (0x00f9ec84)
19 0x0050f688 in fearspdemo (+0x10f687) (0x19e22598)
20 0x19e22598 (0x19e27af8)
Comment 2 Tobias Jakobi 2011-02-08 08:09:07 UTC
The problem seems to be that entry->remove() is called twice in certain cases.

Replacing the member function with something like this fixes the crash:
/* Remove any entries currently in the ACP for this kill. */
void
ir_copy_propagation_elements_visitor::kill(kill_entry *k)
{
   bool remove_var;

   foreach_list_safe(node, acp) {
      acp_entry *entry = (acp_entry *)node;
      remove_var = false;

      if (entry->lhs == k->var) {
	 entry->write_mask = entry->write_mask & ~k->write_mask;
	 if (entry->write_mask == 0)
	    remove_var = true;
      }
      if (entry->rhs == k->var) {
	 remove_var = true;
      }

      if (remove_var) entry->remove();
   }

   /* If we were on a list, remove ourselves before inserting */
   if (k->next)
      k->remove();

   this->kills->push_tail(k);
}

However I have no idea if that's the right fix. Eric?
Comment 3 Sven Arvidsson 2011-06-21 04:17:50 UTC
*** Bug 38501 has been marked as a duplicate of this bug. ***
Comment 4 Sven Arvidsson 2011-06-21 04:31:45 UTC
This pass was re-enabled a while back, but Regnum Online and Bioshock (running
in Wine) still suffers from an infinite loop.
Comment 5 Andy Clayton 2011-11-23 22:27:49 UTC
I've seen this exact same issue with games in wine getting stuck in an infinite loop in libglsl. Commenting out the opt_copy_propagation_elements pass allows them to work again.
Comment 6 Andy Clayton 2011-11-27 22:06:51 UTC
(In reply to comment #5)
> I've seen this exact same issue with games in wine getting stuck in an infinite
> loop in libglsl. Commenting out the opt_copy_propagation_elements pass allows
> them to work again.

Oh, since a version could be helpful: this was as of the xorg edgers PPA version 7.12.0~git20111125.dbf00812-0ubuntu0sarvatt~oneiric (up to dbf00812b073bd3f17cbbaadb14c6f603b83cd10).
Comment 7 Andy Clayton 2011-11-28 05:49:08 UTC
I can't say I know very well what I am looking at it, so I may be completely wrong, but it appears as if this could be some accidental interaction with do_swizzle_swizzle. I stepped through do_common_optimization a number of times, and that is what seems to keep triggering it to loop again. 

Wine-dbg>
1: progress = 0
1038	   progress = do_swizzle_swizzle(ir) || progress;
Wine-dbg>
1: progress = 1
1039	   progress = do_noop_swizzle(ir) || progress;
Wine-dbg>
Comment 8 Andy Clayton 2011-11-28 20:00:05 UTC
Created attachment 53924 [details]
a shader triggering the infinite loop

I've attached a shader that appears to trigger the infinite loop. Simply creating the shader, setting the source, compiling, creating and attaching to a program, and linking the program is enough to trigger it.
Comment 9 Andy Clayton 2011-12-15 21:12:11 UTC
Created attachment 54480 [details]
IR states that do_swizzle_swizzle flips between

I've checked and commenting out do_swizzle_swizzle does also fix the issue. It seems to continually flip the ir between two states. The full ir dumps are attached from _mesa_print_ir, and here is the assign where it continually adds and removes a swiz wwww:

state 1:
      (assign  (xyzw) (var_ref _ret_val@107)  (expression vec4 max (expression vec4 min (swiz wwww (var_ref R0) )(constant float (1.000000)) ) (constant float (0.000000)) ) ) 

state 2:
      (assign  (xyzw) (var_ref _ret_val@108)  (expression vec4 max (expression vec4 min (swiz wwww (swiz wwww (var_ref R0) ))(constant float (1.000000)) ) (constant float (0.000000)) ) )
Comment 10 Andy Clayton 2012-01-03 21:35:06 UTC
Created attachment 55097 [details] [review]
potential patch

I'm attaching a potential patch that fixes what might be a bug inopt_copy_propagation_elements.cpp. As it is now handle_rvalue will be called for both a swizzle and the swizzle's value. However handle_rvalue is written to handle both when passed a swizzle, so calling it only with the swizzle seems correct. If someone who knows better what is going on could review this I would much appreciate it.
Comment 11 Andy Clayton 2012-01-03 21:41:34 UTC
Sorry for the excessive messages, though I forgot to add:

The proposed patch fixes the loop. It is copy_propagation_elements that adds the duplicate swizzle I mentioned in #9. So when handle_rvalue is passed (var_ref R0), it replaces it with (swiz wwww (var_ref R0)). Then swizzle_swizzle condenses it back to the single swizzle, and repeat.
Comment 12 Sven Arvidsson 2012-01-05 14:33:11 UTC
Thanks for giving this bug some lovin'! 

The patch works for me, both Regnum Online and Bioshock no longer hangs on load. I haven't tried the FEAR demo yet.
Comment 13 Sven Arvidsson 2012-01-05 16:34:50 UTC
I also made a piglit run with the patch applied and there are no regressions found. 

Any chance you can post the patch to the mailing list to get it reviewed and included for the upcoming release?
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Comment 14 Andy Clayton 2012-01-05 19:28:25 UTC
Sent, thanks so much for the testing and encouragement. Since it is my first mesa patch it is comforting to know it works for you and doesn't break any tests before sending it off.
Comment 15 Eric Anholt 2012-01-09 13:19:59 UTC
Thanks!  I've pushed your patch:

commit 6c29452f38dacace4f234e9526bfdc1e23fb5051
Author: Andy Clayton <q3aiml@gmail.com>
Date:   Thu Jan 5 21:17:53 2012 -0600

    glsl: fix glsl optimization infinite loop from copy_propagation_elements

And sent out a piglit testcase that's a trimmed-down version of the shader you had here.  That was really great to have for trying to understand this bug.