Matt Turner <matts...@gmail.com> writes:
> On Fri, Mar 4, 2016 at 8:49 PM, Francisco Jerez <curroje...@riseup.net> wrote:
>> Matt Turner <matts...@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.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
