Bug 90081 - [llvmpipe] piglit ext_transform_feedback-immediate-reuse-uniform-buffer regression
Summary: [llvmpipe] piglit ext_transform_feedback-immediate-reuse-uniform-buffer regre...
Status: NEW
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/llvmpipe (show other bugs)
Version: 10.6
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords: bisected, regression
Depends on:
Blocks:
 
Reported: 2015-04-18 02:05 UTC by Vinson Lee
Modified: 2018-04-13 10:24 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vinson Lee 2015-04-18 02:05:38 UTC
mesa: 1eac3ae1a6ebecf353054d937dd603a11ea33fb3 (master 10.6.0-devel)

$ ./bin/ext_transform_feedback-immediate-reuse-uniform-buffer -auto
Probe color at (16,0)
  Expected: 0.062500 0.937500 0.062500
  Observed: 0.000000 0.000000 0.000000
Probe color at (32,0)
  Expected: 0.125000 0.875000 0.125000
  Observed: 0.000000 0.000000 0.000000
Probe color at (48,0)
  Expected: 0.187500 0.812500 0.187500
  Observed: 0.000000 0.000000 0.000000
Probe color at (64,0)
  Expected: 0.250000 0.750000 0.250000
  Observed: 0.000000 0.000000 0.000000
Probe color at (80,0)
  Expected: 0.312500 0.687500 0.312500
  Observed: 0.000000 0.000000 0.000000
Probe color at (96,0)
  Expected: 0.375000 0.625000 0.375000
  Observed: 0.000000 0.000000 0.000000
Probe color at (112,0)
  Expected: 0.437500 0.562500 0.437500
  Observed: 0.000000 0.000000 0.000000
Probe color at (128,0)
  Expected: 0.500000 0.500000 0.500000
  Observed: 0.000000 0.000000 0.000000
Probe color at (144,0)
  Expected: 0.562500 0.437500 0.562500
  Observed: 0.000000 0.000000 0.000000
Probe color at (160,0)
  Expected: 0.625000 0.375000 0.625000
  Observed: 0.000000 0.000000 0.000000
Probe color at (176,0)
  Expected: 0.687500 0.312500 0.687500
  Observed: 0.000000 0.000000 0.000000
Probe color at (192,0)
  Expected: 0.750000 0.250000 0.750000
  Observed: 0.000000 0.000000 0.000000
Probe color at (208,0)
  Expected: 0.812500 0.187500 0.812500
  Observed: 0.000000 0.000000 0.000000
Probe color at (224,0)
  Expected: 0.875000 0.125000 0.875000
  Observed: 0.000000 0.000000 0.000000
Probe color at (240,0)
  Expected: 0.937500 0.062500 0.937500
  Observed: 0.000000 0.000000 0.000000
PIGLIT: {"result": "fail" }

586536a4e1c34725b3b38c3425db569fac0c91e9 is the first bad commit
commit 586536a4e1c34725b3b38c3425db569fac0c91e9
Author: Roland Scheidegger <sroland@vmware.com>
Date:   Thu Apr 9 00:49:11 2015 +0200

    gallivm: don't use control flow when doing indirect constant buffer lookups
    
    llvm goes crazy when doing that, using way more memory and time, though there's
    probably more to it - this points to a very much similar issue as fixed in
    8a9f5ecdb116d0449d63f7b94efbfa8b205d826f. In any case I've seen a quite
    plain looking vertex shader with just ~50 simple tgsi instructions (but with a
    dozen or so such indirect constant buffer lookups) go from a terribly high
    ~440ms compile time (consuming 25MB of memory in the process) down to a still
    awful ~230ms and 13MB with this fix (with llvm 3.3), so there's still obvious
    improvements possible (but I have no clue why it's so slow...).
    The resulting shader is most likely also faster (certainly seemed so though
    I don't have any hard numbers as it may have been influenced by compile times)
    since generally fetching constants outside the buffer range is most likely an
    app error (that is we expect all indices to be valid).
    It is possible this fixes some mysterious vertex shader slowdowns we've seen
    ever since we are conforming to newer apis at least partially (the main draw
    loop also has similar looking conditionals which we probably could do without -
    if not for the fetch at least for the additional elts condition.)
    
    v2: use static vars for the fake bufs, minor code cleanups
    
    Reviewed-by: Jose Fonseca <jfonseca@vmware.com>

:040000 040000 3d9c36f85b3f7cf81413a06ea0454a4c035ea447 9d2ff2be20f10d6f129bc3e1a9979d2a97a37a79 M	src
bisect run success
Comment 1 Roland Scheidegger 2015-04-18 02:29:06 UTC
Ah yes I actually knew about that but didn't consider it all that important.
The reason is that the code was already buggy and this change just exposed the bug.

In short here's what happens:
- the transform feedback buffer is only 4 bytes, which is later bound as constant buffer
- constant buffers are expected to be in multiples of 16 bytes (as the contents are vec4 elements), thus the size is considered to be zero by (llvm) draw (and would be the same for llvmpipe fragment shaders as well).

Before this change, this worked because we do not verify that an access to a buffer constant is within range of the buffer, IFF the access is done via an immediate (so, indirect access would always have returned zero even with the old code). This is in fact not very safe (I've got some quickly hacked up piglit test which tries to cause a crash but didn't manage really more than valgrind warnings). This did not change (though I think it probably will in the future), however for simplified shader code we now bind (1-element sized) fake buffers explicitly if the buffer size is zero (this was actually trivial to crash before just don't bind a constant buffer then any attempt at constant buffer lookup would have produced a nice clean null pointer dereference).

I don't have time to fix these issues though for now. Well the one not verifying accesses outside of range ought to be easy but doesn't help this case, but the bug causing one I'm actually not quite sure HOW it should be fixed. Since it is not really ok to bind 4-byte sized buffers as a constant buffer which thus could be considered a state tracker bug but I'm not sure if it actually could do much about it. Of course could be easily hacked up in draw so the test passes again (could only use fake buffer if size is 0 before the divide by 16 for instance) but it wouldn't really serve much purpose, I'd rather have it fail so the issue doesn't get lost completely.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.