On Thu, Dec 04, 2014 at 05:17:05PM -0800, Kenneth Graunke wrote: > On Thursday, December 04, 2014 03:37:17 PM Ben Widawsky wrote: > > The GS has an interesting use for mul. It's essentially used as a fancy mov > > (in > > fact, I am not sure why a mov isn't used). The documentation in the > > function has > > a very good explanation from Paul on the mechanics. > > > > CHV has some quirks with regard to multiplication. While the documentation > > is > > somewhat unclear, I've found that demoting the src1 operand in the GS mul > > solves > > all the problems. I'd ask that any potential reviewer ignore the other > > instances > > of mul for now (I have more patches), and simply make sure that what this > > patch > > does is correct. > > (probably don't need review suggestions in the commit message; these normally > go below the --- line) >
Thanks. I'm still not used to being able to push my own patches. The --- line was never of any use before. > > This fixes around 2000 piglit tests on BSW. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84777 (with many > > dupes) > > Cc: "10.4" <mesa-sta...@lists.freedesktop.org> > > Cc: "10.4 10.3 10.2" <mesa-sta...@lists.freedesktop.org> > Is there a timeframe for getting this patch in? I would like to spend a few more days working with the validation team to find out exactly why this works, since they tell me it shouldn't fix anything. I don't think it hurts to move forward with this patch, it fixes problems, but it's possible we'll end up with a different fix after. > > Signed-off-by: Ben Widawsky <b...@bwidawsk.net> > > > > --- > > src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 5 ++++- > > src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 3 ++- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > > b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > > index b353539..4f60797 100644 > > --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > > @@ -534,8 +534,11 @@ vec4_generator::generate_gs_set_write_offset(struct > > brw_reg dst, > > brw_push_insn_state(p); > > brw_set_default_access_mode(p, BRW_ALIGN_1); > > brw_set_default_mask_control(p, BRW_MASK_DISABLE); > > + assert(src1.file == BRW_IMMEDIATE_VALUE && > > + src1.type == BRW_REGISTER_TYPE_UD && > > + src1.dw1.ud <= SHRT_MAX); > > Don't you want USHRT_MAX? (I don't think it'll matter in practice.) > > > brw_MUL(p, suboffset(stride(dst, 2, 2, 1), 3), stride(src0, 8, 2, 4), > > - src1); > > + retype(src1, BRW_REGISTER_TYPE_UW)); > > Please add: > > assert(brw->gen >= 7); > > to this function, since UD * UW is illegal on Sandybridge: > > "When multiplying a DW and a W, the W has to be on src0, and the DW has to be > on src1." > > Which is fine, since Sandybridge doesn't run this code. Gen7+ works like you > expect, so I think this is a good change to make. > > > brw_set_default_access_mode(p, BRW_ALIGN_16); > > brw_pop_insn_state(p); > > } > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp > > b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp > > index db0e6cc..2d7ae5a 100644 > > --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp > > @@ -388,7 +388,8 @@ vec4_gs_visitor::emit_control_data_bits() > > */ > > src_reg per_slot_offset(this, glsl_type::uint_type); > > emit(SHR(dst_reg(per_slot_offset), dword_index, 2u)); > > - emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, per_slot_offset, 1u); > > + emit(GS_OPCODE_SET_WRITE_OFFSET, mrf_reg, per_slot_offset, > > + src_reg(1u)); > > This hunk looks unnecessary (please drop it). > This is C++ fail for me. I got lost when trying to figure out how the immediate value is preserved, and understood how src_reg actually worked. I can certainly drop this. > > } > > > > if (urb_write_flags & BRW_URB_WRITE_USE_CHANNEL_MASKS) { > > With those changes, > Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> Thanks. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
