On Fri, Jun 5, 2015 at 4:47 PM, Francisco Jerez <curroje...@riseup.net> wrote:
> Matt Turner <matts...@gmail.com> writes:
>
>> On Thu, Jun 4, 2015 at 9:05 AM, Francisco Jerez <curroje...@riseup.net>
>> wrote:
>>> ---
>>> src/mesa/drivers/dri/i965/brw_fs.cpp | 9 ++---
>>> src/mesa/drivers/dri/i965/brw_fs.h | 3 +-
>>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 57
>>> ++++++++++++++--------------
>>> 3 files changed, 35 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index e55998c..4c1aa60 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -2918,13 +2918,12 @@ fs_visitor::emit_repclear_shader()
>>> int base_mrf = 1;
>>> int color_mrf = base_mrf + 2;
>>>
>>> - fs_inst *mov = emit(MOV(vec4(brw_message_reg(color_mrf)),
>>> - fs_reg(UNIFORM, 0, BRW_REGISTER_TYPE_F)));
>>> - mov->force_writemask_all = true;
>>> + fs_inst *mov = bld.exec_all().MOV(vec4(brw_message_reg(color_mrf)),
>>> + fs_reg(UNIFORM, 0,
>>> BRW_REGISTER_TYPE_F));
>>>
>>> fs_inst *write;
>>> if (key->nr_color_regions == 1) {
>>> - write = emit(FS_OPCODE_REP_FB_WRITE);
>>> + write = bld.emit(FS_OPCODE_REP_FB_WRITE);
>>> write->saturate = key->clamp_fragment_color;
>>> write->base_mrf = color_mrf;
>>> write->target = 0;
>>> @@ -2933,7 +2932,7 @@ fs_visitor::emit_repclear_shader()
>>> } else {
>>> assume(key->nr_color_regions > 0);
>>> for (int i = 0; i < key->nr_color_regions; ++i) {
>>> - write = emit(FS_OPCODE_REP_FB_WRITE);
>>> + write = bld.emit(FS_OPCODE_REP_FB_WRITE);
>>> write->saturate = key->clamp_fragment_color;
>>> write->base_mrf = base_mrf;
>>> write->target = i;
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
>>> b/src/mesa/drivers/dri/i965/brw_fs.h
>>> index de37298..571d411 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>>> @@ -330,7 +330,8 @@ public:
>>> void setup_color_payload(fs_reg *dst, fs_reg color, unsigned components,
>>> unsigned exec_size, bool use_2nd_half);
>>> void emit_alpha_test();
>>> - fs_inst *emit_single_fb_write(fs_reg color1, fs_reg color2,
>>> + fs_inst *emit_single_fb_write(const brw::fs_builder &bld,
>>> + fs_reg color1, fs_reg color2,
>>> fs_reg src0_alpha, unsigned components,
>>> unsigned exec_size, bool use_2nd_half =
>>> false);
>>> void emit_fb_writes();
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>>> index 5d6d6d6..98429fa 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>>> @@ -1296,12 +1296,12 @@ fs_visitor::emit_dummy_fs()
>>> /* Everyone's favorite color. */
>>> const float color[4] = { 1.0, 0.0, 1.0, 0.0 };
>>> for (int i = 0; i < 4; i++) {
>>> - emit(MOV(fs_reg(MRF, 2 + i * reg_width, BRW_REGISTER_TYPE_F,
>>> - dispatch_width), fs_reg(color[i])));
>>> + bld.MOV(fs_reg(MRF, 2 + i * reg_width, BRW_REGISTER_TYPE_F,
>>> + dispatch_width), fs_reg(color[i]));
>>> }
>>>
>>> fs_inst *write;
>>> - write = emit(FS_OPCODE_FB_WRITE);
>>> + write = bld.emit(FS_OPCODE_FB_WRITE);
>>> write->eot = true;
>>> if (devinfo->gen >= 6) {
>>> write->base_mrf = 2;
>>> @@ -1479,7 +1479,7 @@ fs_visitor::setup_color_payload(fs_reg *dst, fs_reg
>>> color, unsigned components,
>>> fs_reg tmp = vgrf(glsl_type::vec4_type);
>>> assert(color.type == BRW_REGISTER_TYPE_F);
>>> for (unsigned i = 0; i < components; i++) {
>>> - inst = emit(MOV(offset(tmp, i), offset(color, i)));
>>> + inst = bld.MOV(offset(tmp, i), offset(color, i));
>>> inst->saturate = true;
>>> }
>>> color = tmp;
>>> @@ -1550,15 +1550,14 @@ fs_visitor::emit_alpha_test()
>>> }
>>>
>>> fs_inst *
>>> -fs_visitor::emit_single_fb_write(fs_reg color0, fs_reg color1,
>>> +fs_visitor::emit_single_fb_write(const fs_builder &bld,
>>> + fs_reg color0, fs_reg color1,
>>> fs_reg src0_alpha, unsigned components,
>>> unsigned exec_size, bool use_2nd_half)
>>> {
>>> assert(stage == MESA_SHADER_FRAGMENT);
>>> brw_wm_prog_data *prog_data = (brw_wm_prog_data*) this->prog_data;
>>> brw_wm_prog_key *key = (brw_wm_prog_key*) this->key;
>>> -
>>> - this->current_annotation = "FB write header";
>>> int header_size = 2, payload_header_size;
>>>
>>> /* We can potentially have a message length of up to 15, so we have to
>>> set
>>> @@ -1589,22 +1588,23 @@ fs_visitor::emit_single_fb_write(fs_reg color0,
>>> fs_reg color1,
>>>
>>> if (payload.aa_dest_stencil_reg) {
>>> sources[length] = fs_reg(GRF, alloc.allocate(1));
>>> - emit(MOV(sources[length],
>>> - fs_reg(brw_vec8_grf(payload.aa_dest_stencil_reg, 0))));
>>> + bld.exec_all().annotate("FB write stencil/AA alpha")
>>> + .MOV(sources[length],
>>> + fs_reg(brw_vec8_grf(payload.aa_dest_stencil_reg, 0)));
>>> length++;
>>> }
>>>
>>> prog_data->uses_omask =
>>> prog->OutputsWritten & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK);
>>> if (prog_data->uses_omask) {
>>> - this->current_annotation = "FB write oMask";
>>> assert(this->sample_mask.file != BAD_FILE);
>>> /* Hand over gl_SampleMask. Only lower 16 bits are relevant. Since
>>> * it's unsinged single words, one vgrf is always 16-wide.
>>> */
>>> sources[length] = fs_reg(GRF, alloc.allocate(1),
>>> BRW_REGISTER_TYPE_UW, 16);
>>> - emit(FS_OPCODE_SET_OMASK, sources[length], this->sample_mask);
>>> + bld.exec_all().annotate("FB write oMask")
>>> + .emit(FS_OPCODE_SET_OMASK, sources[length], this->sample_mask);
>>> length++;
>>> }
>>>
>>> @@ -1665,20 +1665,21 @@ fs_visitor::emit_single_fb_write(fs_reg color0,
>>> fs_reg color1,
>>> if (payload.dest_depth_reg)
>>> sources[length++] = fs_reg(brw_vec8_grf(payload.dest_depth_reg, 0));
>>>
>>> + const fs_builder ubld = bld.group(exec_size, use_2nd_half);
>>> fs_inst *load;
>>> fs_inst *write;
>>> if (devinfo->gen >= 7) {
>>> /* Send from the GRF */
>>> fs_reg payload = fs_reg(GRF, -1, BRW_REGISTER_TYPE_F, exec_size);
>>> - load = emit(LOAD_PAYLOAD(payload, sources, length,
>>> payload_header_size));
>>> + load = ubld.LOAD_PAYLOAD(payload, sources, length,
>>> payload_header_size);
>>
>> I don't see that the LOAD_PAYLOAD had force_sechalf set before. Same
>> comment elsewhere in this patch.
>>
>> What's going on? If this is a functional change... you need to write a
>> commit message!
>
> It's neither an accident nor a functional change. As I mentioned in the
> commit message of PATCH 14, the builder requires you to be explicit
> about the subset of channel enables you want to affect your instructions
> (or call exec_all() if you don't want any at all) if you are planning to
> emit any instruction of non-native SIMD width. Otherwise the assertion
> in fs_builder::emit() fires. This is not a functional change because we
> don't emit the FB writes under non-uniform control flow (we can't
> because of EOT), and because the kill-pixel mask is always sent as a
> header rather than via predication on HSW and up for dual-source writes,
> so always using the first quarter was probably okay, by pure luck.
Seems reasonable to me. A quick sentence about the incidental bug-fix
wouldn't hurt though.
> I can repeat basically the same justification as in PATCH 14 in the
> commit message of this patch, if you'd like it to be more clear.
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
