[prev in list] [next in list] [prev in thread] [next in thread]
List: mesa3d-dev
Subject: Re: [Mesa-dev] [PATCH 10/25] i965: Simplify generator code for untyped surface messages.
From: Paul Berry <stereotype441 () gmail ! com>
Date: 2013-12-31 16:39:38
Message-ID: CA+yLL67tF-FVZKm4BchwdT6-=e__viEFV4Q0moRSPQM6gAqqew () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On 2 December 2013 11:39, Francisco Jerez <currojerez@riseup.net> wrote:
> ---
> src/mesa/drivers/dri/i965/brw_fs.h | 9 ------
> src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 38
> ++++------------------
> src/mesa/drivers/dri/i965/brw_vec4.h | 9 ------
> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 40
> ++++--------------------
> 4 files changed, 12 insertions(+), 84 deletions(-)
>
The commit message should note that you've also loosened the restrictions
on atomic operations--the surf_index used with an atomic operatoin no
longer needs to be an immediate constant.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index 4ada075..7bfa9fd 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -601,15 +601,6 @@ private:
> struct brw_reg offset,
> struct brw_reg value);
>
> - void generate_untyped_atomic(fs_inst *inst,
> - struct brw_reg dst,
> - struct brw_reg atomic_op,
> - struct brw_reg surf_index);
> -
> - void generate_untyped_surface_read(fs_inst *inst,
> - struct brw_reg dst,
> - struct brw_reg surf_index);
> -
> void patch_discard_jumps_to_fb_writes();
>
> struct brw_context *brw;
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 4eb651f..0d50051 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -1255,36 +1255,6 @@ fs_generator::generate_shader_time_add(fs_inst
> *inst,
> }
>
> void
> -fs_generator::generate_untyped_atomic(fs_inst *inst, struct brw_reg dst,
> - struct brw_reg atomic_op,
> - struct brw_reg surf_index)
> -{
> - assert(atomic_op.file == BRW_IMMEDIATE_VALUE &&
> - atomic_op.type == BRW_REGISTER_TYPE_UD &&
> - surf_index.file == BRW_IMMEDIATE_VALUE &&
> - surf_index.type == BRW_REGISTER_TYPE_UD);
> -
> - brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),
> - surf_index, atomic_op.dw1.ud,
> - inst->mlen, true);
> -
> - brw_mark_surface_used(&c->prog_data.base, surf_index.dw1.ud);
> -}
> -
> -void
> -fs_generator::generate_untyped_surface_read(fs_inst *inst, struct brw_reg
> dst,
> - struct brw_reg surf_index)
> -{
> - assert(surf_index.file == BRW_IMMEDIATE_VALUE &&
> - surf_index.type == BRW_REGISTER_TYPE_UD);
> -
> - brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),
> - surf_index, inst->mlen, 1);
> -
> - brw_mark_surface_used(&c->prog_data.base, surf_index.dw1.ud);
> -}
> -
> -void
> fs_generator::generate_code(exec_list *instructions)
> {
> int last_native_insn_offset = p->next_insn_offset;
> @@ -1709,11 +1679,15 @@ fs_generator::generate_code(exec_list
> *instructions)
> break;
>
> case SHADER_OPCODE_UNTYPED_ATOMIC:
> - generate_untyped_atomic(inst, dst, src[0], src[1]);
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], src[1].dw1.ud, inst->mlen, true);
>
I've seen the pattern crop up several times in this series now of: 1.
assert that a brw_reg is an immediate value, 2. extract reg.dw1.ud. How
about if we make a function to do this (maybe brw_get_imm_ud())? I think
that will be a lot more readable since it won't force people to remember
that the immediate value is stored in brw_reg::dw1.
> break;
>
> case SHADER_OPCODE_UNTYPED_SURFACE_READ:
> - generate_untyped_surface_read(inst, dst, src[0]);
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], inst->mlen, src[1].dw1.ud);
> break;
>
In both of the above two cases, generate_untyped_{atomic,surface_read} used
to call brw_mark_surface_used(). That's not being called anymore.
>
> case FS_OPCODE_SET_SIMD4X2_OFFSET:
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> index 355d497..7e07929 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -666,15 +666,6 @@ private:
> void generate_unpack_flags(vec4_instruction *inst,
> struct brw_reg dst);
>
> - void generate_untyped_atomic(vec4_instruction *inst,
> - struct brw_reg dst,
> - struct brw_reg atomic_op,
> - struct brw_reg surf_index);
> -
> - void generate_untyped_surface_read(vec4_instruction *inst,
> - struct brw_reg dst,
> - struct brw_reg surf_index);
> -
> struct brw_context *brw;
>
> struct brw_compile *p;
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> index 3ac45a9..d29c3dd 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> @@ -847,38 +847,6 @@
> vec4_generator::generate_pull_constant_load_gen7(vec4_instruction *inst,
> brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud);
> }
>
> -void
> -vec4_generator::generate_untyped_atomic(vec4_instruction *inst,
> - struct brw_reg dst,
> - struct brw_reg atomic_op,
> - struct brw_reg surf_index)
> -{
> - assert(atomic_op.file == BRW_IMMEDIATE_VALUE &&
> - atomic_op.type == BRW_REGISTER_TYPE_UD &&
> - surf_index.file == BRW_IMMEDIATE_VALUE &&
> - surf_index.type == BRW_REGISTER_TYPE_UD);
> -
> - brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),
> - surf_index, atomic_op.dw1.ud,
> - inst->mlen, true);
> -
> - brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud);
> -}
> -
> -void
> -vec4_generator::generate_untyped_surface_read(vec4_instruction *inst,
> - struct brw_reg dst,
> - struct brw_reg surf_index)
> -{
> - assert(surf_index.file == BRW_IMMEDIATE_VALUE &&
> - surf_index.type == BRW_REGISTER_TYPE_UD);
> -
> - brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),
> - surf_index, inst->mlen, 1);
> -
> - brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud);
> -}
> -
> /**
> * Generate assembly for a Vec4 IR instruction.
> *
> @@ -1193,11 +1161,15 @@
> vec4_generator::generate_vec4_instruction(vec4_instruction *instruction,
> break;
>
> case SHADER_OPCODE_UNTYPED_ATOMIC:
> - generate_untyped_atomic(inst, dst, src[0], src[1]);
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], src[1].dw1.ud, inst->mlen, true);
> break;
>
> case SHADER_OPCODE_UNTYPED_SURFACE_READ:
> - generate_untyped_surface_read(inst, dst, src[0]);
> + assert(src[1].file == BRW_IMMEDIATE_VALUE);
> + brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),
> + src[0], inst->mlen, src[1].dw1.ud);
> break;
>
Same problem about brw_mark_surface_used() here.
With those issues addressed, this patch is:
Reviewed-by: Paul Berry <stereotype441@gmail.com>
[Attachment #5 (text/html)]
<div dir="ltr">On 2 December 2013 11:39, Francisco Jerez <span dir="ltr"><<a \
href="mailto:currojerez@riseup.net" \
target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div \
class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">---<br> \
src/mesa/drivers/dri/i965/brw_fs.h | 9 ------<br> \
src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 38 ++++------------------<br> \
src/mesa/drivers/dri/i965/brw_vec4.h | 9 ------<br> \
src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 40 ++++--------------------<br> 4 \
files changed, 12 insertions(+), 84 \
deletions(-)<br></blockquote><div><br></div><div>The commit message should note that \
you've also loosened the restrictions on atomic operations--the surf_index used \
with an atomic operatoin no longer needs to be an immediate constant.<br> </div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h \
b/src/mesa/drivers/dri/i965/brw_fs.h<br> index 4ada075..7bfa9fd 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs.h<br>
@@ -601,15 +601,6 @@ private:<br>
struct brw_reg offset,<br>
struct brw_reg value);<br>
<br>
- void generate_untyped_atomic(fs_inst *inst,<br>
- struct brw_reg dst,<br>
- struct brw_reg atomic_op,<br>
- struct brw_reg surf_index);<br>
-<br>
- void generate_untyped_surface_read(fs_inst *inst,<br>
- struct brw_reg dst,<br>
- struct brw_reg surf_index);<br>
-<br>
void patch_discard_jumps_to_fb_writes();<br>
<br>
struct brw_context *brw;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp \
b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp<br> index 4eb651f..0d50051 \
100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp<br>
@@ -1255,36 +1255,6 @@ fs_generator::generate_shader_time_add(fs_inst *inst,<br>
}<br>
<br>
void<br>
-fs_generator::generate_untyped_atomic(fs_inst *inst, struct brw_reg dst,<br>
- struct brw_reg atomic_op,<br>
- struct brw_reg surf_index)<br>
-{<br>
- assert(atomic_op.file == BRW_IMMEDIATE_VALUE &&<br>
- atomic_op.type == BRW_REGISTER_TYPE_UD &&<br>
- surf_index.file == BRW_IMMEDIATE_VALUE &&<br>
- surf_index.type == BRW_REGISTER_TYPE_UD);<br>
-<br>
- brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),<br>
- surf_index, atomic_op.dw1.ud,<br>
- inst->mlen, true);<br>
-<br>
- brw_mark_surface_used(&c->prog_data.base, surf_index.dw1.ud);<br>
-}<br>
-<br>
-void<br>
-fs_generator::generate_untyped_surface_read(fs_inst *inst, struct brw_reg dst,<br>
- struct brw_reg surf_index)<br>
-{<br>
- assert(surf_index.file == BRW_IMMEDIATE_VALUE &&<br>
- surf_index.type == BRW_REGISTER_TYPE_UD);<br>
-<br>
- brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),<br>
- surf_index, inst->mlen, 1);<br>
-<br>
- brw_mark_surface_used(&c->prog_data.base, surf_index.dw1.ud);<br>
-}<br>
-<br>
-void<br>
fs_generator::generate_code(exec_list *instructions)<br>
{<br>
int last_native_insn_offset = p->next_insn_offset;<br>
@@ -1709,11 +1679,15 @@ fs_generator::generate_code(exec_list *instructions)<br>
break;<br>
<br>
case SHADER_OPCODE_UNTYPED_ATOMIC:<br>
- generate_untyped_atomic(inst, dst, src[0], src[1]);<br>
+ assert(src[1].file == BRW_IMMEDIATE_VALUE);<br>
+ brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),<br>
+ src[0], src[1].dw1.ud, inst->mlen, \
true);<br></blockquote><div><br></div><div>I've seen the pattern crop up several \
times in this series now of: 1. assert that a brw_reg is an immediate value, 2. \
extract reg.dw1.ud. How about if we make a function to do this (maybe \
brw_get_imm_ud())? I think that will be a lot more readable since it won't force \
people to remember that the immediate value is stored in brw_reg::dw1.<br> \
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> break;<br>
<br>
case SHADER_OPCODE_UNTYPED_SURFACE_READ:<br>
- generate_untyped_surface_read(inst, dst, src[0]);<br>
+ assert(src[1].file == BRW_IMMEDIATE_VALUE);<br>
+ brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),<br>
+ src[0], inst->mlen, src[1].dw1.ud);<br>
break;<br></blockquote><div><br></div><div>In both of the above two cases, \
generate_untyped_{atomic,surface_read} used to call brw_mark_surface_used(). \
That's not being called anymore.<br></div><div> </div> <blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <br>
case FS_OPCODE_SET_SIMD4X2_OFFSET:<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h \
b/src/mesa/drivers/dri/i965/brw_vec4.h<br> index 355d497..7e07929 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
@@ -666,15 +666,6 @@ private:<br>
void generate_unpack_flags(vec4_instruction *inst,<br>
struct brw_reg dst);<br>
<br>
- void generate_untyped_atomic(vec4_instruction *inst,<br>
- struct brw_reg dst,<br>
- struct brw_reg atomic_op,<br>
- struct brw_reg surf_index);<br>
-<br>
- void generate_untyped_surface_read(vec4_instruction *inst,<br>
- struct brw_reg dst,<br>
- struct brw_reg surf_index);<br>
-<br>
struct brw_context *brw;<br>
<br>
struct brw_compile *p;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp \
b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp<br> index 3ac45a9..d29c3dd \
100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp<br>
@@ -847,38 +847,6 @@ \
vec4_generator::generate_pull_constant_load_gen7(vec4_instruction *inst,<br> \
brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud);<br> }<br>
<br>
-void<br>
-vec4_generator::generate_untyped_atomic(vec4_instruction *inst,<br>
- struct brw_reg dst,<br>
- struct brw_reg atomic_op,<br>
- struct brw_reg surf_index)<br>
-{<br>
- assert(atomic_op.file == BRW_IMMEDIATE_VALUE &&<br>
- atomic_op.type == BRW_REGISTER_TYPE_UD &&<br>
- surf_index.file == BRW_IMMEDIATE_VALUE &&<br>
- surf_index.type == BRW_REGISTER_TYPE_UD);<br>
-<br>
- brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),<br>
- surf_index, atomic_op.dw1.ud,<br>
- inst->mlen, true);<br>
-<br>
- brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud);<br>
-}<br>
-<br>
-void<br>
-vec4_generator::generate_untyped_surface_read(vec4_instruction *inst,<br>
- struct brw_reg dst,<br>
- struct brw_reg surf_index)<br>
-{<br>
- assert(surf_index.file == BRW_IMMEDIATE_VALUE &&<br>
- surf_index.type == BRW_REGISTER_TYPE_UD);<br>
-<br>
- brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),<br>
- surf_index, inst->mlen, 1);<br>
-<br>
- brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud);<br>
-}<br>
-<br>
/**<br>
* Generate assembly for a Vec4 IR instruction.<br>
*<br>
@@ -1193,11 +1161,15 @@ vec4_generator::generate_vec4_instruction(vec4_instruction \
*instruction,<br> break;<br>
<br>
case SHADER_OPCODE_UNTYPED_ATOMIC:<br>
- generate_untyped_atomic(inst, dst, src[0], src[1]);<br>
+ assert(src[1].file == BRW_IMMEDIATE_VALUE);<br>
+ brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf),<br>
+ src[0], src[1].dw1.ud, inst->mlen, true);<br>
break;<br>
<br>
case SHADER_OPCODE_UNTYPED_SURFACE_READ:<br>
- generate_untyped_surface_read(inst, dst, src[0]);<br>
+ assert(src[1].file == BRW_IMMEDIATE_VALUE);<br>
+ brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf),<br>
+ src[0], inst->mlen, src[1].dw1.ud);<br>
break;<br></blockquote><div><br></div><div>Same problem about \
brw_mark_surface_used() here.<br><br></div><div>With those issues addressed, this \
patch is:<br><br>Reviewed-by: Paul Berry <<a \
href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br> \
</div></div></div></div>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic