[Mesa-dev] [PATCH] i965/fs: Replace subreg_offset with brw_reg's subnr.

[Iago Toral]

On Thu, 2016-02-11 at 19:12 -0800, Kenneth Graunke wrote:
> On Thursday, February 11, 2016 5:49:55 PM PST Matt Turner wrote:
> > On Thu, Feb 11, 2016 at 3:33 PM, Francisco Jerez <currojerez at riseup.net> 
> wrote:
> > > Would be really nice if we could also get rid of reg_offset as we're at
> > > it.  reg and subreg_offset basically represent the same thing but with
> > > different units, couldn't we just have a single offset field in bytes?
> > > Should it be part of brw_reg or backend_reg?  I think I would lean
> > > towards backend_reg.  In that case does it make sense to move this into
> > > brw_reg now only to move it back to backend_reg later on?
> > 
> > That would be nice.
> >
> > I'm just not sure how to do it. brw_reg has to have the subnr field,
> > and it's nice if that's the field the higher levels use too.
> > 
> > I wonder -- is it possible that we could just get rid of reg_offset
> > too? For gathering data we have load_payload, so it's not useful
> > there. I think it's mainly useful for accessing elements of texturing
> > results. Is doubt there is a way we could avoid that though?
> 
> I disagree - I don't think this would be nice at all.
> 
> When we designed the IR, we needed something to handle cases like
> texturing results, where we actually need to store a whole vec4,
> and can't break it into separate scalar components.  (Note that
> messages used MRFs; we didn't even know about send-from-GRF.)
> 
> To handle this, Eric created an abstraction of "virtual registers with
> size > 1", where we basically have an array of registers, each of which
> holds a single scalar value.  It can be thought of as a vecN, and
> reg_offset is the array index - v[i] - selecting each of the logically
> contiguous components.  reg_offset only makes sense on these large
> virtual registers - it has no meaning for real hardware registers.
> 
> In contrast, subreg_offset (which came later) offered a way to access
> particular channels of a single hardware register, such as g0.2.  We
> added this later to make the IR more expressive.
> 
> While these are both offsets, they serve very different purposes.
> 
> Replacing subreg_offset with subnr makes a lot of sense to me, as
> both are basically a way to provide a byte offset for the start of
> a register region, allowing unaligned register access.  But reg_offset
> is a different beast.
> 
> If you want to be rid of it, then perhaps we should consider removing
> the "VGRF of size > 1" abstraction.  One could imagine a system where
> we allocate separate VRFs for each scalar value, but record that
> "VRFs 4, 12, and 63 need to be contiguous", passing that information
> to the register allocator.  There are certainly other fine approaches.
> 
> I would also humbly request that you wait until FP64 lands before making
> any major changes.  It's very easy to conflate type size, number of SIMD
> channels, and register offsets, and a lot of the FP64 work is fixing
> places that are confused about that.  I'd really like to avoid making
> our Igalia friends' lives harder by making them rebase 100 patches on
> IR redesigns.

Thanks Ken for thinking about us :) I would definitely appreciate this!
The patch count for fp64 is already above 150 and rebasing it on top of
changes like these sounds quite scary to me.

Iago