Bug 86469

This should actually be pretty easy to fix.  Sandybridge has more than 16 MRFs, so we could just adjust the spilling code to use the higher numbered ones which aren't used for anything else.

The check also seems a bit heavy handed...it may not actually conflict...

(In reply to Kenneth Graunke from comment #1)
> This should actually be pretty easy to fix.  Sandybridge has more than 16
> MRFs, so we could just adjust the spilling code to use the higher numbered
> ones which aren't used for anything else.
> 
> The check also seems a bit heavy handed...it may not actually conflict...

I've checked that Effects works on SNB by simply setting spill_base_mrf 15 (as 14 is used). I did not find documentation that would indicate more than 16 MRFs available though. Docs indicate there are 16 from which 1 .. 15 can be used.

(In reply to Tapani Pälli from comment #2)
> (In reply to Kenneth Graunke from comment #1)
> > This should actually be pretty easy to fix.  Sandybridge has more than 16
> > MRFs, so we could just adjust the spilling code to use the higher numbered
> > ones which aren't used for anything else.
> > 
> > The check also seems a bit heavy handed...it may not actually conflict...
> 
> I've checked that Effects works on SNB by simply setting spill_base_mrf 15
> (as 14 is used). I did not find documentation that would indicate more than
> 16 MRFs available though. Docs indicate there are 16 from which 1 .. 15 can
> be used.

SNB PRM Vol4 Part2's "Table 5-4. MRF Registers Available in Device Hardware" says "Number per Thread" - "24 registers"

This would be an easy experiment for someone with a Sandybridge -- modify the fs_visitor::spill_reg() function in src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp

(In reply to Matt Turner from comment #3)
> (In reply to Tapani Pälli from comment #2)
> > (In reply to Kenneth Graunke from comment #1)
> > > This should actually be pretty easy to fix.  Sandybridge has more than 16
> > > MRFs, so we could just adjust the spilling code to use the higher numbered
> > > ones which aren't used for anything else.
> > > 
> > > The check also seems a bit heavy handed...it may not actually conflict...
> > 
> > I've checked that Effects works on SNB by simply setting spill_base_mrf 15
> > (as 14 is used). I did not find documentation that would indicate more than
> > 16 MRFs available though. Docs indicate there are 16 from which 1 .. 15 can
> > be used.
> 
> SNB PRM Vol4 Part2's "Table 5-4. MRF Registers Available in Device Hardware"
> says "Number per Thread" - "24 registers"
> 
> This would be an easy experiment for someone with a Sandybridge -- modify
> the fs_visitor::spill_reg() function in
> src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp

Yep, I saw this but bspec states that it might not be safe to use MRFs other than 1 .. 15. It feels a bit risky to go change this just because of one demo.

(In reply to Tapani Pälli from comment #4)
> Yep, I saw this but bspec states that it might not be safe to use MRFs other
> than 1 .. 15.

Where does it say that?

(In reply to Matt Turner from comment #5)
> (In reply to Tapani Pälli from comment #4)
> > Yep, I saw this but bspec states that it might not be safe to use MRFs other
> > than 1 .. 15.
> 
> Where does it say that?

SNB bspec, EU Overview -> Messages -> Message Register File (MRF), last paragraph.

(In reply to Tapani Pälli from comment #6)
> SNB bspec, EU Overview -> Messages -> Message Register File (MRF), last
> paragraph.

That page doesn't say "it's not safe". If anything it says MRFs > 15 don't exist ("There are 16 MRF registers").

It wouldn't surprise me if the registers don't exist and the "24" is a mistake, but it also wouldn't surprise me if SNB actually has 24 MRFs and they just didn't update a bunch of documentation. Experimentation is the only way to be sure.

(In reply to Matt Turner from comment #7)
> (In reply to Tapani Pälli from comment #6)
> > SNB bspec, EU Overview -> Messages -> Message Register File (MRF), last
> > paragraph.
> 
> That page doesn't say "it's not safe". If anything it says MRFs > 15 don't
> exist ("There are 16 MRF registers").
> 
> It wouldn't surprise me if the registers don't exist and the "24" is a
> mistake, but it also wouldn't surprise me if SNB actually has 24 MRFs and
> they just didn't update a bunch of documentation. Experimentation is the
> only way to be sure.

I dont have access right now but the last clause is something like "regardless of the actual hw implementation thread should not assume registers above 16 can be used" but yep, one can experiment

(In reply to Tapani Pälli from comment #8)
> I dont have access right now but the last clause is something like
> "regardless of the actual hw implementation thread should not assume
> registers above 16 can be used" but yep, one can experiment

I seem to recall at least having started that experiment at some point.  I think I gave up because the generator asserts 16 MRFs all over the place and I didn't want to bother special-casing SNB all the way down.

(In reply to Tapani Pälli from comment #8)
> I dont have access right now but the last clause is something like
> "regardless of the actual hw implementation thread should not assume
> registers above 16 can be used" but yep, one can experiment

I still don't think you're understanding what I'm saying. The statement you're referring to says

> Regardless of actual hardware implementation, the thread should not assume that MRF addresses above m15 wrap to legal MRF registers.

My point is that you cannot draw any conclusions about MRF > 15 from a page that claims they do not exist.

(In reply to Matt Turner from comment #10)
> (In reply to Tapani Pälli from comment #8)
> > I dont have access right now but the last clause is something like
> > "regardless of the actual hw implementation thread should not assume
> > registers above 16 can be used" but yep, one can experiment
> 
> I still don't think you're understanding what I'm saying. The statement
> you're referring to says
> 
> > Regardless of actual hardware implementation, the thread should not assume that MRF addresses above m15 wrap to legal MRF registers.
> 
> My point is that you cannot draw any conclusions about MRF > 15 from a page
> that claims they do not exist.

Ok, sorry if this caused confusion, I just thought to answer that this is why I stopped experimentation with this bug, I was not sure if it is safe and worth the effort.

(In reply to Tapani Pälli from comment #2)
> (In reply to Kenneth Graunke from comment #1)
> > This should actually be pretty easy to fix.  Sandybridge has more than 16
> > MRFs, so we could just adjust the spilling code to use the higher numbered
> > ones which aren't used for anything else.
> > 
> > The check also seems a bit heavy handed...it may not actually conflict...
> 
> I've checked that Effects works on SNB by simply setting spill_base_mrf 15
> (as 14 is used). I did not find documentation that would indicate more than
> 16 MRFs available though. Docs indicate there are 16 from which 1 .. 15 can
> be used.

Even if we keep 16 as the limit for now, why not compute spill_base_mrf based on the actual data in mrf_used? Something like this:

for (int i = BRW_MAX_MRF - 1; !mrf_used[i]; i--);
spill_base_mrf = i + 1;
if (spill_base_mrf >= BRW_MAX_MRF) {
   fail("Register spilling not supported with m%d used", i);
   return;
}

We would have to do this in spill_reg and emit_spill, but that would make sure that we don't fail if there enough unused MRFs available within that limit, which is what is happening here.

Just to mention that I tested it on my SNB laptop with today's master (800efb0690e962750b9a072bcbab279fdaae24a1). It runs the demo very slowly but without spilling errors.

I ran git-bisect. This is the commit that fixed it:

commit 9b577d57029bb643f2b48b80648b4f901818e93b
Author: Matt Turner <mattst88@gmail.com>
Date:   Mon Mar 16 21:33:31 2015 -0700

    glsl: Transform pow(x, 4) into (x*x)*(x*x).
    
    Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

However we still have this issue in SNB and we could hit it again in other programs (I have not checked current status of related bugs like bug #90631 and bug #91259).

Iago's proposal seems reasonable, What do you think about it?

*** Bug 91259 has been marked as a duplicate of this bug. ***

(In reply to Matt Turner from comment #10)
> (In reply to Tapani Pälli from comment #8)
> > I dont have access right now but the last clause is something like
> > "regardless of the actual hw implementation thread should not assume
> > registers above 16 can be used" but yep, one can experiment
> 
> I still don't think you're understanding what I'm saying. The statement
> you're referring to says
> 
> > Regardless of actual hardware implementation, the thread should not assume that MRF addresses above m15 wrap to legal MRF registers.
> 
> My point is that you cannot draw any conclusions about MRF > 15 from a page
> that claims they do not exist.
(In reply to Matt Turner from comment #7)
> (In reply to Tapani Pälli from comment #6)
> > SNB bspec, EU Overview -> Messages -> Message Register File (MRF), last
> > paragraph.
> 
> That page doesn't say "it's not safe". If anything it says MRFs > 15 don't
> exist ("There are 16 MRF registers").
> 
> It wouldn't surprise me if the registers don't exist and the "24" is a
> mistake, but it also wouldn't surprise me if SNB actually has 24 MRFs and
> they just didn't update a bunch of documentation. Experimentation is the
> only way to be sure.


FWIW, I've sent a RFC series to the mailing list that implements this test:
http://lists.freedesktop.org/archives/mesa-dev/2015-September/094584.html

Spoiler alert: SNB supports 24 MRF registers

This should be fixed in master since:

commit 5d23ce2f15bda866990750b49d7860144dff2e68
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Thu Sep 17 13:43:52 2015 +0200

    i965/vec4: Use MRF registers 21-23 for spilling in gen6
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

commit 6789a32075774fc332eb7432910c7fbc21ee2026
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Tue Sep 15 16:33:48 2015 +0200

    i965/fs: Use MRF registers 21-23 for spilling in gen6
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

    Iago, thanks for the patches and Kenneth for the review! And to all
   for the fruitfull discussion. But I'm still wondering that nobody from
   intel could promptly definitely approve the assumptions about the
   number of registers. Or the documentation has been in meanwhile
   updated?

(In reply to Andrey Gursky from comment #17)
>     Iago, thanks for the patches and Kenneth for the review! And to all
>    for the fruitfull discussion. But I'm still wondering that nobody from
>    intel could promptly definitely approve the assumptions about the
>    number of registers. Or the documentation has been in meanwhile
>    updated?

The documentation has not been updated, what we did was to test if we could use 24 MRFs, which was easy enough to do. SNB has been out there for many years, so I imagine it might not be that easy to find the right person inside Intel to confirm something as specific as this.

Any chance the fix could be backported in Mesa 11?

(In reply to Iago Toral from comment #16)
> This should be fixed in master since:
> 
> commit 5d23ce2f15bda866990750b49d7860144dff2e68
> Author: Iago Toral Quiroga <itoral@igalia.com>
> Date:   Thu Sep 17 13:43:52 2015 +0200

-> Marking the bug as fixed.

(In reply to Eero Tamminen from comment #20)
> (In reply to Iago Toral from comment #16)
> > This should be fixed in master since:
> > 
> > commit 5d23ce2f15bda866990750b49d7860144dff2e68
> > Author: Iago Toral Quiroga <itoral@igalia.com>
> > Date:   Thu Sep 17 13:43:52 2015 +0200
> 
> -> Marking the bug as fixed.


Is this FIXED? Bug is still marked as NEW.

(In reply to Juan A. Suarez from comment #21)
> (In reply to Eero Tamminen from comment #20)
> > (In reply to Iago Toral from comment #16)
> > > This should be fixed in master since:
> > > 
> > > commit 5d23ce2f15bda866990750b49d7860144dff2e68
> > > Author: Iago Toral Quiroga <itoral@igalia.com>
> > > Date:   Thu Sep 17 13:43:52 2015 +0200
> > 
> > -> Marking the bug as fixed.
> 
> 
> Is this FIXED? Bug is still marked as NEW.

Eero forgot to flip the state when he updated the bug report. Marking as fixed now.