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

[Matt Turner]

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.