Bug 62696

Summary:	[r300g, bisected] Around 50 piglit vs variable-indexing tests fails after "glsl_to_tgsi: allocate arrays separately v2"
Product:	Mesa	Reporter:	Pavel Ondračka <pavel.ondracka>
Component:	Drivers/Gallium/r300	Assignee:	Default DRI bug account <dri-devel>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	ckoenig.leichtzumerken
Version:	git	Keywords:	regression
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	RADEON_DEBUG=fp,vp output with latest mesa master RADEON_DEBUG=fp,vp output before Possible fix RADEON_DEBUG=fp,vp from vs-uniform-array-mat2-col-row-rd test Alternative fix.

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.