[Mesa-dev] [PATCH 01/38] i965/fs: Introduce FS IR builder.

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Mon, Jun 8, 2015 at 9:50 AM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Fri, Jun 5, 2015 at 4:13 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Matt Turner <mattst88 at gmail.com> writes:
>>>> On Fri, Jun 5, 2015 at 1:42 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>> Matt Turner <mattst88 at gmail.com> writes:
>>>>>> On Thu, Jun 4, 2015 at 9:04 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>>>>> +      /**
>>>>>>> +       * Gen4 predicated IF.
>>>>>>> +       */
>>>>>>> +      instruction *
>>>>>>> +      IF(brw_predicate predicate) const
>>>>>>> +      {
>>>>>>> +         instruction *inst = emit(BRW_OPCODE_IF);
>>>>>>> +         return set_predicate(predicate, inst);
>>>>>>> +      }
>>>>>>> +
>>>>>>> +      /**
>>>>>>> +       * Gen6 IF with embedded comparison.
>>>>>>> +       */
>>>>>>> +      instruction *
>>>>>>> +      IF(const src_reg &src0, const src_reg &src1,
>>>>>>> +         brw_conditional_mod condition) const
>>>>>>> +      {
>>>>>>> +         assert(shader->devinfo->gen == 6);
>>>>>>> +         return set_condmod(condition,
>>>>>>> +                            emit(BRW_OPCODE_IF,
>>>>>>> +                                 null_reg_d(),
>>>>>>> +                                 fix_unsigned_negate(src0),
>>>>>>> +                                 fix_unsigned_negate(src1)));
>>>>>>
>>>>>> I don't see that we were calling resolve_ud_negate() on
>>>>>> fs_visitor::emit_if_gen6.
>>>>>>
>>>>> Right, but shouldn't any instruction comparing unsigned values which are
>>>>> potentially negated call resolve_ud_negate()?
>>>>
>>>> From looking at the Sandybridge PRM, the IF with embedded conditional
>>>> cannot do source modifiers, so I think the answer is no.
>>>>
>>> I suspect that the PRM is wrong, IIRC I checked this on real hardware
>>> and the simulator, and the embedded conditional on SNB behaves largely
>>> like the normal CMP instruction, source modifiers work as usual (though
>>> we didn't actually make use of them AFAIK).
>>
>> All the more reason that this should be done as a separate patch with
>> an explanation.
>>
>>>>>> In fact, we've removed the function entirely (a known regression in
>>>>>> the switch to NIR). I'd drop this for now. It's unused as well.
>>>>>>
>>>>> If there are still plans to restore this functionality, I would rather
>>>>> leave it alone rather than remove it now for somebody else to have to
>>>>> rewrite it later on, because it's less work and doesn't seem to hurt.
>>>>
>>>> But moreover, again I don't like hidden changes like this in what is
>>>> sold as a refactor.
>>>>
>>> It was never my intention to sell this as a refactor.
>>
>> Are you just arguing semantics?
>>
>> Call it what you will -- the series is refactoring a bunch of code.
>
> Ok, so "sold as a refactor" might not have been the best phrase.
> However, this does involve a refactor; it just also involves more
> substantive changes.  Those substatnive changes should be in separate
> commits preferably either before or after *all* of the code that does
> the refactor.  Why?  Three reasons:
>
>  1) It's logically cleaner.  The code does three things: add a
> builder, use the builder, fix bugs.  Those should be three separate
> commits or sets of commits.
>
>  2) It's much easier to review.  If the reader knows going into things
> that adding the builder is a direct copy+paste and the only changes
> involved are to pull things like force_sechalf from the builder then
> it's much easier for them to verify.  If they have to go through it
> with a fine-toothed comb looking for subtle (or not so subtle)
> differences, then doing so on that much code is exhausting.  It's much
> easier if the substantive changes are broken out and labeled as such
> in the commit message.  As it stands, you completely rewrote a
> function and changed its semantics with no commentary whatsoever as to
> what you changed and why or, for that matter, even that you changed
> it.  This makes review *much* more difficult and makes it easier for
> bugs to slip through.
>
I've argued elsewhere why the rewrite of that function didn't change the
already relied-upon semantics of the original function, it was strictly
an extension and happened mostly by accident.

> 3) It's more debuggable.  Let's say, hypothetically, that your change
> to the gen6 if code introduces a bug.  Someone files the bug, and I
> look at it in bugzilla.  The first thing I do is bisect.  The first
> bad commit is labled "i965/fs: Migrate translation of NIR control flow
> to the IR builder."  From both the commit message and the diff itself,
> that commit looks like a no-op.  I know that we migrated something
> from A to B so I go start looking at the two emit if implementations.
> Maybe I start with a git blame.  That takes me to a megacommit that,
> again, looks like it should be exactly the same as the original.  It
> has no useful information that anything is different.  So I now
> compare the two functions themselves.  That's still not good because
> the resolve_ud_negate function they're calling are different and there
> is no guarantee that the difference between the two emit if functions
> are the actual problem.
>
> Yes, giant refactors are messy, painful to review, and hard to debug.
> Please don't make it any worse than it has to be.

You seem to be missing the fact that the builder was a rewrite from the
start.  While I generally agree with the benefits you describe, to the
point that if you had asked for it politely I would have considered
doing a pile of diff-magic to make the rewrite seem like a
copy-and-paste refactor (which it was not) followed by a number of
mostly trivial changes, you must admit that it would have involved a
considerable amount of work, of the order of work-hours -- It's by no
means obvious whether the benefits of your approach would have been
compelling enough if you consider the sum of costs for author and
reviewer rather than the reviewer cost alone.  In fact shortly before I
sent this series to the mailing list I verified myself one last time
that the builder was functionally equivalent to the code it replaced.
It took me a matter of minutes.

It's somehow ironic that many of the other review comments I got in this
thread were in the exact opposite direction -- While you're at it,
change the previously used type of this register to allow compaction,
use bld.null_reg_f() instead of the previously used fixed brw_reg (this
one actually led to an unexpected functional regression which was
thankfully caught in time by Jenkins), fix this or that preexisting
codestyle issue.  None of this was a logical dependency of my changes,
where's the line supposed to be?

> --Jason
-------------- 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/20150609/e8ba5944/attachment-0001.sig>