Bug 108624

Summary: [regression][bisected] "nir: Copy propagation between blocks" regression
Product: Mesa Reporter: Alejandro Piñeiro (freenode IRC: apinheiro) <apinheiro>
Component: Drivers/DRI/i965Assignee: Caio Marcelo de Oliveira Filho <caio.oliveira>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: apinheiro, caio.oliveira, jason
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: regression test

Description Alejandro Piñeiro (freenode IRC: apinheiro) 2018-11-01 11:32:00 UTC 
Created attachment 142326 [details]
regression test

Not sure if it is normal to create [regression] bugs for tests that are not on any upstream branch, but as today is a bank day here in Spain, I will not have time for triagging for some days, so I prefer to save it somewhere, in order to not forget about it.

Initially I found it while testing ARB_gl_spirv with borrowed tests (tests from other specs). But it also fails with vulkan, by using vkrunner.

So, attached the ARB_gl_spirv test. Can also be found here:
   https://github.com/Igalia/piglit/tree/apinheiro/copy-propagation-between-blocks-regression

And the vkrunner equivalent here:
   https://github.com/Igalia/vkrunner/tree/apinheiro/copy-propagation-between-blocks-regression

FWIW, the ARB_gl_spirv test is executed as any other piglit shader_test, although it needs to override the extension, so:
  $ export MESA_EXTENSION_OVERRIDE=GL_ARB_gl_spirv 
  $./bin/shader_runner tests/spec/arb_gl_spirv/execution/fs-large-local-array-vec4.shader_test -auto

After bisecting, the candidates for the guilty commits are:

* dc349f07b5f9c257f68ecf0cbb89f9722a42233a ("nir: Take call instruction into account in copy_prop_vars")
* b3c6146925595ec3a7eece3afb9ccaad32906d4c ("nir: Copy propagation between blocks")

I have two candidates because the first commit doesn't build correctly ("error: ‘copies’ undeclared (first use in this function)")

Those commits changes nir_opt_copy_prop_vars among other things. And in fact, if we don't call that optimization (dirty hack just to scope where the problem is), the test goes back to passing (although everything gets slower).
Comment 1 Alejandro Piñeiro (freenode IRC: apinheiro) 2018-11-08 11:19:33 UTC 
> Not sure if it is normal to create [regression] bugs for tests that are not on any upstream branch

Now that vkrunner has been integrated on piglit, I have just sent a series with tests:
https://lists.freedesktop.org/archives/piglit/2018-November/025343.html
Comment 2 Caio Marcelo de Oliveira Filho 2018-12-17 23:32:46 UTC 
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/23 should fix this.
Comment 3 Caio Marcelo de Oliveira Filho 2018-12-19 17:48:36 UTC 
commit 947f7b452a550c66cfb9a8c9518e35635eb25947
Author: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>
Date:   Fri Dec 14 16:10:32 2018 -0800

    nir: properly find the entry to keep in copy_prop_vars
    
    When copy propagation handles a store/copy, it iterates the current
    copy entries to remove aliases, but keeps the "equal" entry (if
    exists) to be updated.
    
    The removal step may swap the entries around (to ensure there are no
    holes), invalidating previous iteration pointers.  The bug was saving
    such pointer to use later.  Change the code to first perform the
    removals and then find the remaining right entry.
    
    This was causing updates to be lost since they were being made to an
    entry that was not part of the current copies.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108624
    Fixes: b3c61469255 "nir: Copy propagation between blocks"
    Cc: mesa-stable@lists.freedesktop.org
    Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

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.