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

[Ian Romanick]

On 03/08/2016 04:47 PM, Francisco Jerez wrote:
> 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).

FB reads will be necessary for GL_KHR_blend_equation_advanced which is
needed for OpenGL ES 3.2.

Implementing full memory dependency tracking is hard.  It seems like we
can do a little bit better than we're doing, and we will need to do
better for GL_KHR_blend_equation_advanced.  Most shaders won't read the
framebuffer, and those that do should only have FB reads and writes
conflict.

I think having a thing like Matt's is_scheduling_barrier is probably the
right idea in this sort of world, but I think it's to narrow.  I may be
wrong, but I don't think GLSL requires that memory accessible through
multiple methods is coherent.  That is, a write to an SSBO could be
reordered after a read from a UBO.  If those happen to the be same
backing storage, that's tough.  This is like the strict aliasing rules in C.

There's really three things that we care about.  There are things that
"significantly" affect program flow (i.e., HALT and EOT), things that
are explicitly barriers (e.g., atomics, explicit memory barriers), and
things that write the kind of memory object you are reading.  It seems
like we ought to be able to capture that in a small set of functions
that make it obvious what's happening... and give us some room to
improve the infrastructure.

Maybe the compromise for now is to:

 - Leave has_side_effects as is, but maybe change it's name to
   something_memory_something.

 - Add a new method that returns true for FS_OPCODE_PLACEHOLDER_HALT or
   eot.  No current opinion on what to call it... maybe
   something_thread_end_something.

 - Add is_scheduling_barrier that returns true if
   something_memory_something or something_thread_end_something.

Or maybe I have no idea what I'm talking about and should be ignored. :)
Always possible...

> 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.
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160308/33004fc7/attachment.sig>