[Mesa-dev] [PATCH 09/10] i965: Add and use is_scheduling_barrier() function.

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Fri, Mar 4, 2016 at 8:49 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Matt Turner <mattst88 at gmail.com> writes:
>>
>>> Though there is a lot of overlap with has_side_effects(), these do mean
>>> different things.
>>
>> Can we do it the other way around and implement is_scheduling_barrier()
>> in terms of has_side_effects()?  has_side_effects() seems like the more
>> fundamental of the two and because is_scheduling_barrier() is specific
>> to the scheduler it would make more sense to keep it static inline in
>> brw_schedule_instructions.cpp for the sake of encapsulation.
>>
>> AFAIUI is_scheduling_barrier() is merely a makeshift approximation at
>> missing memory dependency analysis, and in the long term is the wrong
>> question to ask (IMHO the right question is "does this instruction have
>> an execution dependency with respect to this other?", which implies that
>> either of the two instructions has some sort of side effect, but not the
>> converse).  has_side_effects() OTOH has a well-defined answer that can
>> be answered by looking at the semantics of the instruction alone,
>> independent from scheduling heuristics and surrounding compiler
>> infrastructure.
>>
>> I think for the moment I'd make is_scheduling_barrier return true if the
>> instruction has side effects, except where you have the guarantee that
>> the side-effectful instruction won't have a (non-dataflow related)
>> execution dependency with any other instruction of the program, which is
>> currently only the case for non-EOT FB_WRITE -- Pretty much has Ken had
>> open-coded it in his scheduling changes except for the non-EOT part.
>
> I think your suggestion is to keep has_side_effects() the same and to
> wrap it with a static is_scheduling_barrier(). What we have is
>
> bool
> backend_instruction::has_side_effects() const
> {
>    switch (opcode) {
>    [list of instructions]
>       return true;
>    default:
>       return false;
>    }
> }
>
> bool
> fs_inst::has_side_effects() const
> {
>    return this->eot || backend_instruction::has_side_effects();
> }
>
> And then in the scheduler,
>
> if ((inst->opcode == FS_OPCODE_PLACEHOLDER_HALT ||
>      inst->has_side_effects()) &&
>     inst->opcode != FS_OPCODE_FB_WRITE)
>    add_barrier_deps(n);
>
> where the FS_OPCODE_FB_WRITE check was added recently to avoid
> treating it as a barrier. That seems pretty dirty, because
> has_side_effects() returns true for that opcode, so we're just hacking
> around that. I noted in my revert that it also had the effect of
> making an FB_WRITE with EOT not a barrier.
>
> So, your suggestion is to add another layer on top of that, that
> checks inst->eot?
>
> We'd have something like
>
> static inline is_scheduling_barrier(fs_inst *inst)
> {
>    return inst->opcode == FS_OPCODE_PLACEHOLDER_HALT ||
>           (inst->has_side_effects() &&
>            inst->opcode != FS_OPCODE_FB_WRITE) ||
>           inst->eot;
> }
>
> where, tracing through the execution, fs_inst::has_side_effects would
> return true because inst->eot, but since opcode is FB_WRITE that
> expression would evaluate to false. But then because inst->eot, it'd
> evaluate to true.
>
> Doesn't that seem *really* hacky?
>
> Can I go ahead and get someone else's opinion? I doubt you're going to agree.

Oh, I definitely agree, it seems a bit of a hack to me -- It seems hacky
because the whole scheduling barrier business IS a hack.  Instead of
implementing actual memory dependency analysis we want to do the
following (at least for the time being) which I think we all agree is a
hack:

 Treat any instruction with side-effects as a scheduling barrier except
 where we have the guarantee that the side-effects of the instruction
 won't have any influence on any other instruction in the program.

That's currently only the case for non-EOT FB_WRITE (and there's nothing
special about FB_WRITE that makes it different from other side-effectful
instructions other than the fact that we currently don't implement
framebuffer reads, so it's pretty much coincidental that they currently
have no influence on anything else).

Your is_scheduling_barrier() proposal is a fairly clear translation of
my sentence explaining what we want to do into code.  It seems like a
hack in code because it is a hack in principle.  AFAICT building
is_scheduling_barrier() into the core IR data structures only obscures
the matter and involves other compiler infrastructure into this hack
while they don't need to know or care.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160308/abb1f6ce/attachment.sig>