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

[Jason Ekstrand]

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.

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.
--Jason