[Mesa-dev] i965 implementation of the ARB_shader_image_load_store built-ins. (v2)

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Tue, May 5, 2015 at 2:17 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Kenneth Graunke <kenneth at whitecape.org> writes:
>>> I like the idea of the builder refactor - having a mechanism for emitting
>>> code at arbitrary points makes a ton of sense.  But I'm uncomfortable
>>> with how much code is added - and duplicated from the existing visitor
>>> code - given that fs_visitor is refactored to use it.
>>>
>>> I agree with Jason that we should discuss our goal - what things should
>>> look like - but frankly, I think we should hold off on large scale
>>> refactoring until after we delete the GLSL IR backend.  There will be
>>> much less code to reorganize.
>>>
>> I agree, I wasn't planning on migrating the rest of the compiler
>> back-end to the builder framework until that happens.
>
> We talked about this on IRC yesterday -- I find it difficult to review
> some of these patches adding large amounts of new infrastructure
> that's only used in other in this series.
>
> Basically, the problem is that you're adding (1) a large amount of new
> infrastructure, and (2) a large amount of tiling calculation/format
> conversion code and neither can be independently evaluated. If the new
> infrastructure was proposed and we could see how it was useful in the
> existing compiler, we could evaluate it. If the new tiling
> calculation/format conversion code used existing compiler
> infrastructure, we could evaluate it.
>
> Combining the two in the same series though makes it difficult.

Matt, I know that a 3k LOC series might be intimidating at first glance,
but:

 - The additional infrastructure is badly needed.  If you've bothered to
   read the commit message [maybe longest in (our git tree's) history]
   or skimmed through the body of the fs_builder patch, read the
   6-screenful e-mail I sent you last October including a detailed
   explanation of the builder, or any of the working sample code
   (including a port of large parts of existing compiler code) I sent
   you back then, you surely already know that I'm not doing this on a
   whim.  The infrastructure equivalent to the builder should have
   *never* been part of the GLSL IR visitor, and the more new
   functionality we throw at the compiler relying on the old
   infrastructure the more difficult the migration will get.

 - Nothing prevents you from "evaluating" the builder framework
   independent from the tiling and format conversion code.  You have
   more than enough information to assess whether the change is
   reasonable on its own.  You have the lengthy explanation from last
   October.  You have sample code from last October showing how it's
   useful in the existing compiler -- And for the case that doesn't seem
   convincing enough I'm attaching a partial port of brw_fs_visitor.cpp
   to the builder framework based on a current tree [but note that it's
   just an example and not meant to be upstreamed].  You have enough
   knowledge of the existing infrastructure that hardly anything about
   the new infrastructure will come as a surprise for you -- It largely
   mimics the existing IR-building methods, their semantics and
   implementation, and the transition from one to the other usually
   amounts to replacing "emit(FOO(...))" with "bld.FOO(...)".  You have
   enough documentation in the code itself -- I've spent a great deal of
   time documenting the builder framework and the image load/store
   implementation [to a much greater extent than is usual in the rest of
   the back-end code, a quickly written bash script gives a
   comment-to-code ratio of 31% vs. the average of 19% in the rest of
   the FS back-end].  Lastly you have me to answer any questions that
   could come up during review -- And the fact that you haven't asked
   any specific technical questions so far other than about codestyle
   and logistics makes me think that you haven't really attempted to
   understand the code in detail yet, and the supposed difficulty you
   talk about may be based on a presumption rather than on your actual
   experience.

 - Even if I turn out to be wrong, your "evaluation" is inconclusive for
   some strange reason, and the new infrastructure is merged with some
   hidden architectural flaw, the risk is close to non-existent.  The
   worst thing that could happen is we have to flip the switch to
   disable ARB_shader_image_load_store again until the problem is fixed.
   No critical functionality is affected because the new infrastructure
   is completely self-contained.

> It seems like we'd be buying into a transition to the new builder
> without having seen what the transition looks like ("[switching to the
> new infrastructure] will be undertaken by a separate series, as it
> involves major back-end surgery").
>
That's not nearly what I meant.  Of course whether and how we carry out
the transition will still be open for discussion, that statement you
quote was intended to show that I'm willing to do the hard work and not
going to abandon the new infrastructure while the transition is half-way
done (as Ken rudely insinuated on IRC).

> That said, I think people are okay with the builder, in principle at
> least. Splitting the visitor classes certainly seems like a good idea.
> In an ideal world, we'd transition the backend over to your new
> builder, and then add the new image_load_store bits on top of that.
> Since we want to delete brw_*_visitor.cpp before we transition to your
> new builder and we're not planning to delete brw_*_visitor.cpp until
> after 10.6, then this can't happen in 10.6 unless we change something.
>
Yeah, I agree that it wouldn't make sense to do the transition until the
GLSL IR visitor is removed, but I don't see anything less "ideal" about
having ARB_shader_image_load_store be the first to use the new
infrastructure.  If anything it's an advantage that the new experimental
infrastructure (although I'm quite confident in it and calling it
experimental is an overstatement) is only exercised by a single
extension we can easily disable rather than by the whole compiler.

>>> It may also make sense to land Skylake support first - I believe we can
>>> use typed surface messages in most places, which is a lot easier.  We
>>> could land that, then add the additional complexity to support BDW/HSW.
>
> I'd like to reiterate this point. The platform we have to support this
> extension on is Gen9, and Gen9's typed messages support nearly all of
> the formats required.
>
> If we enable Gen9, we'll have more time to figure out how to do the
> rest of the infrastructure changes. Is doing Gen9 first feasible?

SKL doesn't fundamentally change the picture.  It still needs a large
portion of the infrastructure introduced in this series, including the
format conversion code (although it will only be fully exercised by a
smaller subset of the formats, thankfully).  The only major thing it
saves us from doing is the tiling calculation, which is abstracted out
in a single (ugly) less than 100 LOC function part of PATCH 19 that
doesn't use the new infrastructure in any particularly clever way.

Because of the amount of infrastructure you'd have to re-implement and
because adding SKL support to the existing series is trivial (the change
required in this whole series adds a single line of code), I think that
starting from scratch with a SKL-only implementation would be a massive
waste of time.

The changes required in my old state-setup series are a bit more
invasive, but they are still quite straightforward and I already have
the extension working at the same level as on earlier generations.  I'll
send updated versions of the affected patches shortly.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: i965-fs_visitor_builder_example.patch
Type: text/x-diff
Size: 72662 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150513/72d0504c/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150513/72d0504c/attachment-0001.sig>