[Mesa-dev] GLSL IR is no longer cool where to from here?

[Jason Ekstrand]

On Fri, Feb 5, 2016 at 6:57 PM, Timothy Arceri <timothy.arceri at collabora.com
> wrote:

> For the past couple of months I've been working away solely in the
> wasteland that is GLSL IR and one things seems clear. No one wants to
> review this code anymore. A lot of the original developers have either
> moved on or are busy with other things.
>
> The difference between sending a patch with nir: ... vs glsl: ... is
> very noticable.
>

I think there is a bit more to that then a lack of coolness.  I have
trouble getting NIR patches reviewed all the time.  One of the factors is
that a large portion of the NIR patches that fly by are just additions to
nir_opt_algebraic which, apart from verifying that the transformation is
correct, take almost no work to review.  Refactor something or start
modifying a more core area like to/from-SSA or a lowering pass and finding
reviewers suddenly becomes a *lot* harder.

Another factor is that a lot of NIR patches are very specifically working
towards some project that the Intel mesa team is working together on.
Since a lot of us see each other on a fairly regular basis, it's pretty
easy to say "Hey, can you review this quick" and things happen.  I think
some of us need to try and be better about looking for things on the list
to review. :-(


> Its not impossible to get reviews for patches, especially if they are a
> small part of a bigger series not just confined to GLSL IR, but
> anything involving a refactoring can be difficult as no-one wants to
> relearn how this code works, step up to the ast code and things are
> even worse.
>

Yes, it's true that a lot of people don't want to re-learn how that code
works.  We've also had a substantial number of people who have gotten into
compiler development post-NIR and have never learned how any of the stuff
above it works.  (I personally fall into that category.)

It's also worth recognizing that you've been getting into some pretty
gnarly stuff with uniforms and things.  That code can be tricky as well as
mysterious.  When you get into tricky code that few people know well,
finding reviews can be hard.  The best thing I know is to find the other
person or two who know it well and ping them.


> So I guess the discussion I'm trying to kick off is, with the CTS, dEPQ
> and piglit all saying no regressions (or even reporting fixes \0/).
> Should one still be forced to go around hassling people for a rubber
> stamped r-b? Or can we relax the criteria for pushing bug-fix/refactor
> type patches for GLSL-IR?
>

Personally, I don't think that's the solution.  Maybe I'm just scared of
the code because I don't know it, but some of your refactors haven't been
trivial (correct me if I'm wrong on that) and bugfixes can get pretty
subtle.  I think review is still probably in order.  At the very least, I'd
rather try and solve the "can't find reviewers" problem before relaxing the
review requirements if we can.


> The other thing I've consider is maybe this I just don't review enough
> patches for people to reciprocate, although I've made an effort to
> review patches where I feel I can since being employed to work on Mesa
> so hopefully thats not it. Maybe it's time for Ken to run his script
> again :-P
>

I don't think that's the problem.  As long as you're being a pretty good
review citizen, you shouldn't have to have a list of people deeply in your
debt in order to get patches reviewed.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160206/551851ea/attachment.html>