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

[Francisco Jerez]

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.

> ---
>  src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp |  6 ++----
>  src/mesa/drivers/dri/i965/brw_shader.cpp                | 12 +++++++++++-
>  src/mesa/drivers/dri/i965/brw_shader.h                  |  2 ++
>  3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp b/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> index 98fa5e3..67b713b 100644
> --- a/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_schedule_instructions.cpp
> @@ -917,9 +917,7 @@ fs_instruction_scheduler::calculate_deps()
>     foreach_in_list(schedule_node, n, &instructions) {
>        fs_inst *inst = (fs_inst *)n->inst;
>  
> -      if (inst->opcode == FS_OPCODE_PLACEHOLDER_HALT ||
> -          inst->is_control_flow() ||
> -          inst->has_side_effects())
> +      if (inst->is_scheduling_barrier() || inst->is_control_flow())
>           add_barrier_deps(n);
>  
>        /* read-after-write deps. */
> @@ -1152,7 +1150,7 @@ vec4_instruction_scheduler::calculate_deps()
>     foreach_in_list(schedule_node, n, &instructions) {
>        vec4_instruction *inst = (vec4_instruction *)n->inst;
>  
> -      if (inst->is_control_flow() || inst->has_side_effects())
> +      if (inst->is_scheduling_barrier() || inst->is_control_flow())
>           add_barrier_deps(n);
>  
>        /* read-after-write deps. */
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> index d007ed0..80673e5 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -879,7 +879,7 @@ backend_instruction::writes_accumulator_implicitly(const struct brw_device_info
>  }
>  
>  bool
> -backend_instruction::has_side_effects() const
> +backend_instruction::is_scheduling_barrier() const
>  {
>     switch (opcode) {
>     case SHADER_OPCODE_UNTYPED_ATOMIC:
> @@ -896,6 +896,7 @@ backend_instruction::has_side_effects() const
>     case SHADER_OPCODE_URB_WRITE_SIMD8_PER_SLOT:
>     case SHADER_OPCODE_URB_WRITE_SIMD8_MASKED:
>     case SHADER_OPCODE_URB_WRITE_SIMD8_MASKED_PER_SLOT:
> +   case FS_OPCODE_PLACEHOLDER_HALT:
>     case FS_OPCODE_FB_WRITE:
>     case SHADER_OPCODE_BARRIER:
>     case TCS_OPCODE_URB_WRITE:
> @@ -907,6 +908,15 @@ backend_instruction::has_side_effects() const
>  }
>  
>  bool
> +backend_instruction::has_side_effects() const
> +{
> +   switch (opcode) {
> +   default:
> +      return is_scheduling_barrier();
> +   }
> +}
> +
> +bool
>  backend_instruction::is_volatile() const
>  {
>     switch (opcode) {
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> index 82374a4..0ec2a84 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.h
> +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> @@ -124,6 +124,8 @@ struct backend_instruction : public exec_node {
>      */
>     bool has_side_effects() const;
>  
> +   bool is_scheduling_barrier() const;
> +
>     /**
>      * True if the instruction might be affected by side effects of other
>      * instructions.
> -- 
> 2.4.10
>
> _______________________________________________
> 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160304/0d39925f/attachment.sig>