Bug 62696 - [r300g, bisected] Around 50 piglit vs variable-indexing tests fails after "glsl_to_tgsi: allocate arrays separately v2"
Summary: [r300g, bisected] Around 50 piglit vs variable-indexing tests fails after "gl...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/r300 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact:
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2013-03-24 13:36 UTC by Pavel Ondračka
Modified: 2013-03-26 10:39 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
RADEON_DEBUG=fp,vp output with latest mesa master (38.26 KB, text/plain)
2013-03-24 13:36 UTC, Pavel Ondračka
Details
RADEON_DEBUG=fp,vp output before (33.25 KB, text/plain)
2013-03-24 13:37 UTC, Pavel Ondračka
Details
Possible fix (1.19 KB, patch)
2013-03-24 15:27 UTC, Christian König
Details | Splinter Review
RADEON_DEBUG=fp,vp from vs-uniform-array-mat2-col-row-rd test (24.72 KB, text/plain)
2013-03-24 17:28 UTC, Pavel Ondračka
Details
Alternative fix. (1.18 KB, text/plain)
2013-03-25 10:03 UTC, Christian König
Details

Description Pavel Ondračka 2013-03-24 13:36:24 UTC
Created attachment 76962 [details]
RADEON_DEBUG=fp,vp output with latest mesa master

first bad commit:
commit 3f67251e3d0ce61a0e7fc16de91de6fb49cad768
Author: Christian König <christian.koenig@amd.com>
Date:   Sun Mar 10 14:33:29 2013 +0100

    glsl_to_tgsi: allocate arrays separately v2
    
    Instead of allocating everything as temporaries, use the
    new array allocation functions.
    
    v2: fix bug in simplify_cmp, declare arrays on demand

example test output:
./bin/shader_runner tests/spec/glsl-1.20/execution/variable-indexing/vs-temp-array-mat2-index-col-row-rd.shader_test -auto
r300: DRM version: 2.29.0, Name: ATI RV530, ID: 0x71c5, GB: 1, Z: 2
r300: GART size: 509 MB, VRAM size: 256 MB
r300: AA compression RAM: YES, Z compression RAM: YES, HiZ RAM: YES
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (10,10)
  Expected: 0.000000 1.000000 0.000000
  Observed: 0.450980 0.549020 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (10,25)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (25,10)
  Expected: 0.000000 1.000000 0.000000
  Observed: 0.450980 0.549020 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (25,25)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (50,10)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (50,25)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (65,10)
  Expected: 0.000000 1.000000 0.000000
  Observed: 0.549020 0.450980 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (65,25)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (90,10)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (90,25)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (105,10)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
fallback: no matching format for GL_RGB, GL_FLOAT
Probe at (105,25)
  Expected: 0.000000 1.000000 0.000000
  Observed: 1.000000 0.000000 0.000000
PIGLIT: {'result': 'fail' }

(RADEON_DEBUG=fp,vp outputs before bad commit and with latest master attached)

GPU:RV530
Mesa:ec9c3882d949298366c872f766d3d18c6ae93f8e
Kernel: 3.8.3-103.fc17.i686
Comment 1 Pavel Ondračka 2013-03-24 13:37:04 UTC
Created attachment 76963 [details]
RADEON_DEBUG=fp,vp output before
Comment 2 Christian König 2013-03-24 15:27:48 UTC
Created attachment 76967 [details] [review]
Possible fix

Please try the attached patch.
Comment 3 Pavel Ondračka 2013-03-24 17:28:18 UTC
Created attachment 76971 [details]
RADEON_DEBUG=fp,vp from vs-uniform-array-mat2-col-row-rd test

(In reply to comment #2)
> Created attachment 76967 [details] [review] [review]
> Possible fix
> 
> Please try the attached patch.

Yes, your patch fixes the vs-temp-array-mat* tests and vs-varying-array-mat* tests. Still failing are some vs-uniform-array-mat* tests.
I'm attaching output from one of those...
Comment 4 Marek Olšák 2013-03-24 20:03:12 UTC
(In reply to comment #2)
> Created attachment 76967 [details] [review] [review]
> Possible fix
> 
> Please try the attached patch.

I thought the array support was backward compatible. If if were true, this patch wouldn't be needed, right?
Comment 5 Christian König 2013-03-25 08:51:34 UTC
(In reply to comment #4)
> I thought the array support was backward compatible. If if were true, this
> patch wouldn't be needed, right?

Yes indeed, and I find that rather strange, too.

Well one thing that I haven't considered before is that when a target doesn't support indirect addressing all indirect accesses are replaced with a chain of "if (i == 0) then return x[0] else if (i == 1) then return x[1].....", so having arrays separated makes this inefficient code even more inefficient (cause the register scavenger then leaves arrays alone), that's what this patch really should fix.

But you're right, it should NEVER result in incorrect code, so we either have a bug in R300g that's triggered by using more registers than necessary (unlikely), or we are still missing something in the glsl_to_tgsi pass (likely).

@Pavel: Could you make sure that the vs-uniform-array-mat2-col-row-rd test is indeed failing with that change? And also please attach a log of the good case for this test.

Thanks in advance,
Christian.
Comment 6 Christian König 2013-03-25 10:03:01 UTC
Created attachment 76987 [details]
Alternative fix.

Ok, I think I've figured out what's going wrong here.

Could you test the attached patch? It's an alternative version of the previous patch (and should NOT be applied on top).

Thanks,
Christian.
Comment 7 Pavel Ondračka 2013-03-25 12:54:50 UTC
(In reply to comment #6)
> Created attachment 76987 [details]
> Alternative fix.
> 
> Ok, I think I've figured out what's going wrong here.
> 
> Could you test the attached patch? It's an alternative version of the
> previous patch (and should NOT be applied on top).
> 
> Thanks,
> Christian.

Yes, the latest patch fixes all regressed tests here. Also no new regressions with quick piglit tests.


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.