[Mesa-dev] [PATCH] i965/fs: initialize src as reg_undef for texture opcodes
Tapani Pälli
tapani.palli at intel.com
Mon Jun 9 21:45:35 PDT 2014
On 06/09/2014 05:34 PM, Robert Bragg wrote:
> Hey Tapani,
>
> I came across this issue the other day too and can at least confirm
> that I wrote an almost identical patch.
>
> I was a bit unsure whether this was the best place to fix this issue
> since it seems a bit unobvious, in isolation, why we emit these
> texture ops with a place holder register, but maybe for others more
> experienced than me with the code, this is fine.
Yep, I was puzzled quite a bit with MRF until Matt explained how things
work. Currently it is quite hard to see that there is src set via MRF as
it is set differently than with payload. Hopefully there wiill be some
refactoring in this area in some point.
> Something I wasn't clear about, was whether the
> gen6_resolve_implied_move() call in brw_SAMPLE is really needed. It
> looks like the fs and vec4 visitors (where we emit these ops) both
> explicitly setup an MRF payload and before 07af0ab both would have
> effectively passed reg_undef registers through to generate_tex() which
> would just result in the _resolve_move() being a no-op that returns a
> register for msg_reg_nr.
>
> As another possible approach; I smoke tested the following change
> which also seemed to fixed this issue for me:
OK, there was also alternative fix to set implied_header in
"fs_generator::generate_tex" in a similar way as
"fs_generator::generate_fb_write" does. Personally I like that src is
initialized as soon as possible because it gets passed around here and
there and for example "fs_generator::generate_code" pretty much assumes
that there is some content there. If someone accidentially uses
uninitialized src (like now did) it is very hard to debug.
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 2fa65e9..899fe47 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -2317,7 +2317,7 @@ void brw_SAMPLE(struct brw_compile *p,
> struct brw_instruction *insn;
>
> if (msg_reg_nr != -1)
> - gen6_resolve_implied_move(p, &src0, msg_reg_nr);
> + src0 = brw_message_reg(msg_reg_nr);
>
> insn = next_insn(p, BRW_OPCODE_SEND);
> insn->header.predicate_control = 0; /* XXX */
>
> I'm not familiar enough with this to know if a change like that would
> be right/acceptable though and haven't had a chance to try and run
> piglit to see if that breaks something I'm not aware of.
>
> --
> Regards,
> Robert
>
>
> On Mon, Jun 9, 2014 at 10:30 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>> Commit 07af0ab changed fs_inst to have 0 sources for texture opcodes
>> in emit_texture_gen5 (Ironlake, Sandybrige) while fs_generator still
>> uses a single source from brw_reg struct. Patch sets src as reg_undef
>> which matches the behavior before the constructor got changed.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79534
>> ---
>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index 0b4fbb7..3089932 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -1171,21 +1171,21 @@ fs_visitor::emit_texture_gen5(ir_texture *ir, fs_reg dst, fs_reg coordinate,
>> fs_inst *inst = NULL; bu
>> switch (ir->op) {
>> case ir_tex:
>> - inst = emit(SHADER_OPCODE_TEX, dst);
>> + inst = emit(SHADER_OPCODE_TEX, dst, reg_undef);
>> break;
>> case ir_txb:
>> mlen = MAX2(mlen, header_present + 4 * reg_width);
>> emit(MOV(fs_reg(MRF, base_mrf + mlen), lod));
>> mlen += reg_width;
>>
>> - inst = emit(FS_OPCODE_TXB, dst);
>> + inst = emit(FS_OPCODE_TXB, dst, reg_undef);
>> break;
>> case ir_txl:
>> mlen = MAX2(mlen, header_present + 4 * reg_width);
>> emit(MOV(fs_reg(MRF, base_mrf + mlen), lod));
>> mlen += reg_width;
>>
>> - inst = emit(SHADER_OPCODE_TXL, dst);
>> + inst = emit(SHADER_OPCODE_TXL, dst, reg_undef);
>> break;
>> case ir_txd: {
>> mlen = MAX2(mlen, header_present + 4 * reg_width); /* skip over 'ai' */
>> @@ -1215,17 +1215,17 @@ fs_visitor::emit_texture_gen5(ir_texture *ir, fs_reg dst, fs_reg coordinate,
>> case ir_txs:
>> emit(MOV(fs_reg(MRF, base_mrf + mlen, BRW_REGISTER_TYPE_UD), lod));
>> mlen += reg_width;
>> - inst = emit(SHADER_OPCODE_TXS, dst);
>> + inst = emit(SHADER_OPCODE_TXS, dst, reg_undef);
>> break;
>> case ir_query_levels:
>> emit(MOV(fs_reg(MRF, base_mrf + mlen, BRW_REGISTER_TYPE_UD), fs_reg(0u)));
>> mlen += reg_width;
>> - inst = emit(SHADER_OPCODE_TXS, dst);
>> + inst = emit(SHADER_OPCODE_TXS, dst, reg_undef);
>> break;
>> case ir_txf:
>> mlen = header_present + 4 * reg_width;
>> emit(MOV(fs_reg(MRF, base_mrf + mlen - reg_width, BRW_REGISTER_TYPE_UD), lod));
>> - inst = emit(SHADER_OPCODE_TXF, dst);
>> + inst = emit(SHADER_OPCODE_TXF, dst, reg_undef);
>> break;
>> case ir_txf_ms:
>> mlen = header_present + 4 * reg_width;
>> @@ -1235,13 +1235,13 @@ fs_visitor::emit_texture_gen5(ir_texture *ir, fs_reg dst, fs_reg coordinate,
>> /* sample index */
>> emit(MOV(fs_reg(MRF, base_mrf + mlen, BRW_REGISTER_TYPE_UD), sample_index));
>> mlen += reg_width;
>> - inst = emit(SHADER_OPCODE_TXF_CMS, dst);
>> + inst = emit(SHADER_OPCODE_TXF_CMS, dst, reg_undef);
>> break;
>> case ir_lod:
>> - inst = emit(SHADER_OPCODE_LOD, dst);
>> + inst = emit(SHADER_OPCODE_LOD, dst, reg_undef);
>> break;
>> case ir_tg4:
>> - inst = emit(SHADER_OPCODE_TG4, dst);
>> + inst = emit(SHADER_OPCODE_TG4, dst, reg_undef);
>> break;
>> default:
>> fail("unrecognized texture opcode");
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list