Francisco Jerez <[email protected]> writes: > Jason Ekstrand <[email protected]> writes: > >> On Sat, May 26, 2018 at 12:15 PM, Francisco Jerez <[email protected]> >> wrote: >> >>> Jason Ekstrand <[email protected]> writes: >>> >>> > On Sat, May 26, 2018 at 11:27 AM, Francisco Jerez <[email protected] >>> > >>> > wrote: >>> > >>> >> Jason Ekstrand <[email protected]> writes: >>> >> >>> >> > Doing instruction header setup in the generator is aweful for a number >>> >> > of reasons. For one, we can't schedule the header setup at all. For >>> >> > another, it means lots of implied writes which the instruction >>> scheduler >>> >> > and other passes can't properly read about. The second isn't a huge >>> >> > problem for FB writes since they always happen at the end. We made a >>> >> > similar change to sampler handling in ff4726077d86. >>> >> > --- >>> >> > src/intel/compiler/brw_fs.cpp | 103 >>> >> ++++++++++++++++++++++++++------ >>> >> > src/intel/compiler/brw_fs_generator.cpp | 66 -------------------- >>> >> > 2 files changed, 86 insertions(+), 83 deletions(-) >>> >> > >>> >> > diff --git a/src/intel/compiler/brw_fs.cpp >>> b/src/intel/compiler/brw_fs. >>> >> cpp >>> >> > index 3e9ccc4..5276a66 100644 >>> >> > --- a/src/intel/compiler/brw_fs.cpp >>> >> > +++ b/src/intel/compiler/brw_fs.cpp >>> >> > @@ -3247,7 +3247,18 @@ fs_visitor::emit_repclear_shader() >>> >> > write->mlen = 1; >>> >> > } else { >>> >> > assume(key->nr_color_regions > 0); >>> >> > + >>> >> > + struct brw_reg header = >>> >> > + retype(brw_message_reg(base_mrf), BRW_REGISTER_TYPE_UD); >>> >> > + bld.exec_all().group(16, 0) >>> >> > + .MOV(header, retype(brw_vec8_grf(0, 0), >>> BRW_REGISTER_TYPE_UD)); >>> >> > + >>> >> > for (int i = 0; i < key->nr_color_regions; ++i) { >>> >> > + if (i > 0) { >>> >> > + bld.exec_all().group(1, 0) >>> >> > + .MOV(component(header, 2), brw_imm_ud(i)); >>> >> > + } >>> >> > + >>> >> > write = bld.emit(FS_OPCODE_REP_FB_WRITE); >>> >> > write->saturate = key->clamp_fragment_color; >>> >> > write->base_mrf = base_mrf; >>> >> > @@ -3972,25 +3983,83 @@ lower_fb_write_logical_send(const fs_builder >>> >> &bld, fs_inst *inst, >>> >> > int header_size = 2, payload_header_size; >>> >> > unsigned length = 0; >>> >> > >>> >> > - /* From the Sandy Bridge PRM, volume 4, page 198: >>> >> > - * >>> >> > - * "Dispatched Pixel Enables. One bit per pixel indicating >>> >> > - * which pixels were originally enabled when the thread was >>> >> > - * dispatched. This field is only required for the end-of- >>> >> > - * thread message and on all dual-source messages." >>> >> > - */ >>> >> > - if (devinfo->gen >= 6 && >>> >> > - (devinfo->is_haswell || devinfo->gen >= 8 || >>> >> !prog_data->uses_kill) && >>> >> > - color1.file == BAD_FILE && >>> >> > - key->nr_color_regions == 1) { >>> >> > - header_size = 0; >>> >> > - } >>> >> > + if (devinfo->gen < 6) { >>> >> > + /* For gen4-5, we always have a header consisting of g0 and g1. >>> >> We have >>> >> > + * an implied MOV from g0,g1 to the start of the message. The >>> >> MOV from >>> >> > + * g0 is handled by the hardware and the MOV from g1 is >>> provided >>> >> by the >>> >> > + * generator. This is required because, on gen4-5, the >>> generator >>> >> may >>> >> > + * generate two write messages with different message lengths >>> in >>> >> order >>> >> > + * to handle AA data properly. >>> >> > + * >>> >> > + * Also, since the pixel mask goes in the g0 portion of the >>> >> message and >>> >> > + * since render target writes are the last thing in the shader, >>> >> we write >>> >> > + * the pixel mask directly into g0 and it will get copied as >>> part >>> >> of the >>> >> > + * implied write. >>> >> > + */ >>> >> > + if (prog_data->uses_kill) { >>> >> > + bld.exec_all().group(1, 0) >>> >> > + .MOV(retype(brw_vec1_grf(0, 0), BRW_REGISTER_TYPE_UW), >>> >> > + brw_flag_reg(0, 1)); >>> >> > + } >>> >> > + >>> >> > + assert(length == 0); >>> >> > + length = 2; >>> >> > + } else if ((devinfo->gen <= 7 && !devinfo->is_haswell && >>> >> > + prog_data->uses_kill) || >>> >> > + color1.file != BAD_FILE || >>> >> > + key->nr_color_regions > 1) { >>> >> > + /* From the Sandy Bridge PRM, volume 4, page 198: >>> >> > + * >>> >> > + * "Dispatched Pixel Enables. One bit per pixel indicating >>> >> > + * which pixels were originally enabled when the thread >>> was >>> >> > + * dispatched. This field is only required for the end-of- >>> >> > + * thread message and on all dual-source messages." >>> >> > + */ >>> >> > + const fs_builder ubld = bld.exec_all().group(8, 0); >>> >> > + >>> >> > + /* The header starts off as g0 and g1 */ >>> >> > + fs_reg header = ubld.vgrf(BRW_REGISTER_TYPE_UD, 2); >>> >> > + ubld.group(16, 0).MOV(header, retype(brw_vec8_grf(0, 0), >>> >> > + BRW_REGISTER_TYPE_UD)); >>> >> > + >>> >> > + uint32_t g00_bits = 0; >>> >> > + >>> >> > + /* Set "Source0 Alpha Present to RenderTarget" bit in message >>> >> > + * header. >>> >> > + */ >>> >> > + if (inst->target > 0 && key->replicate_alpha) >>> >> > + g00_bits |= 1 << 11; >>> >> > + >>> >> > + /* Set computes stencil to render target */ >>> >> > + if (prog_data->computed_stencil) >>> >> > + g00_bits |= 1 << 14; >>> >> > + >>> >> > + if (g00_bits) { >>> >> > + /* OR extra bits into g0.0 */ >>> >> > + ubld.group(1, 0).OR(component(header, 0), >>> >> > + retype(brw_vec1_grf(0, 0), >>> >> > + BRW_REGISTER_TYPE_UD), >>> >> > + brw_imm_ud(g00_bits)); >>> >> > + } >>> >> > + >>> >> > + /* Set the render target index for choosing BLEND_STATE. */ >>> >> > + if (inst->target > 0) { >>> >> > + ubld.group(1, 0).MOV(component(header, 2), >>> >> brw_imm_ud(inst->target)); >>> >> > + } >>> >> > + >>> >> > + if (prog_data->uses_kill) { >>> >> > + ubld.group(1, 0).MOV(retype(component(header, 15), >>> >> > + BRW_REGISTER_TYPE_UW), >>> >> > + brw_flag_reg(0, 1)); >>> >> > + } >>> >> >>> >> The original patch from my branch implemented the header setup in a >>> >> helper function called setup_fb_write_header() which allowed it to share >>> >> the code among emit_repclear_shader() and lower_fb_write_logical_send(). >>> >> I don't think there is any benefit from open-coding the header setup in >>> >> the already complex enough callers, seems like an obfuscation to me >>> >> (particularly of the emit_repclear_shader() codepath with PATCH 52 in >>> >> mind). >>> >> >>> > >>> > Yeah, I think this one could go either way, TBH. Having it in a helper >>> > avoids some setup in emit_repclear_shader (though not that much) and is >>> > nice because it's sort-of functional in nature. The thing I like about >>> > this approach is that it combines the decision to use the header with the >>> > code to create said header. We could do that in a helper I suppose and >>> > have it return BAD_FILE if no header is needed. >>> > >>> >>> Yes, a helper could combine the decision of whether to send a header or >>> not pretty easily. >>> >>> > Also, the "allocate and return a register" model ended up resulting in >>> > worse code on gen4-5 because it didn't play nicely with the implied MRF >>> > write. On those platforms, it's easier to set the header up by doing the >>> > needed MOVs to g0 and letting the implied MRF write copy it into the >>> > message. >>> > >>> >>> I don't think there's much of a performance benefit from an implied MRF >>> write relative to a favorably scheduled single-GRF MOV on Gen4-5. >>> Actually under some conditions it can take more EU cycles because you >>> eat the latency of the implied MOV without being able to schedule >>> anything in between. >>> >> >> The bigger problem is that we either have to keep the impled MRF write or >> we have to handle the AA data workaround in the visitor as well. >> Ultimately, that's probably what we want to do. I'm trying to avoid >> debugging gen4... >> > > That's no big deal, the implied MRF write can probably stay for the > moment,
In the "runtime_check_aads_emit == true" case that is.
> whether the header is set up in the visitor or in the generator.
> The question is just whether the source of the implied move will be r0
> or the first payload register of the FB write.
>
>>
>>> > As far as complexity goes, I'm not sure. Setting up FB writes is an
>>> > inherently complex operation. Probably the biggest thing I didn't like
>>> > about having it be in a helper was that the header code was around 7-800
>>> > lines away from the code to set up the rest of the FB write. That could
>>> > have been solved by a pre-declaration, however.
>>> >
>>>
>>> It's a couple of keystrokes away if your editor supports some sort of
>>> search function. And I appreciate the possibility not to see the header
>>> setup code while dealing with the FB write lowering code if it's not
>>> what I'm looking for. And the possibility to share code which my
>>> original patch took advantage of.
>>>
>>> > You are free to continue to disagree with my approach. Hopefully, I've
>>> at
>>> > least convinced you that I didn't do it just because I like straight
>>> inline
>>> > code (which I do like).
>>> >
>>>
>>> Ugh... I was just thinking you had inlined it in order to
>>> micro-optimize it, but knowing you actually prefer a bunch of hairy code
>>> over a single call of a well-behaved abstraction (it was indeed
>>> referentially transparent and close to a pure function) to achieve the
>>> same effect makes me even more shocked ;P
>>>
>>
>> We have always had different definitions of code cleanliness. :-)
>>
>
> Didn't ever think we disagreed on structured programming being useful to
> write code which is easily maintainable and reusable... :P
>
>> We can switch it back to being a function that returns a register though
>> it'll take a little reworking of gen4 as mentioned above. I don't think it
>> really saves us anything but whatever...
>>
>> --Jason
>>
>>
>>> > --Jason
>>> >
>>> >
>>> >> >
>>> >> > - if (header_size != 0) {
>>> >> > - assert(header_size == 2);
>>> >> > - /* Allocate 2 registers for a header */
>>> >> > - length += 2;
>>> >> > + assert(length == 0);
>>> >> > + sources[0] = header;
>>> >> > + sources[1] = horiz_offset(header, 8);
>>> >> > + length = 2;
>>> >> > }
>>> >> > + assert(length == 0 || length == 2);
>>> >> > + header_size = length;
>>> >> >
>>> >> > if (payload.aa_dest_stencil_reg) {
>>> >> > sources[length] = fs_reg(VGRF, bld.shader->alloc.allocate(1));
>>> >> > diff --git a/src/intel/compiler/brw_fs_generator.cpp
>>> >> b/src/intel/compiler/brw_fs_generator.cpp
>>> >> > index 8cc9d31..1a2b961 100644
>>> >> > --- a/src/intel/compiler/brw_fs_generator.cpp
>>> >> > +++ b/src/intel/compiler/brw_fs_generator.cpp
>>> >> > @@ -307,9 +307,6 @@ fs_generator::fire_fb_write(fs_inst *inst,
>>> >> > void
>>> >> > fs_generator::generate_fb_write(fs_inst *inst, struct brw_reg
>>> payload)
>>> >> > {
>>> >> > - struct brw_wm_prog_data *prog_data = brw_wm_prog_data(this->prog_
>>> >> data);
>>> >> > - const brw_wm_prog_key * const key = (brw_wm_prog_key * const)
>>> >> this->key;
>>> >> > -
>>> >> > if (devinfo->gen < 8 && !devinfo->is_haswell) {
>>> >> > brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
>>> >> > }
>>> >> > @@ -320,69 +317,6 @@ fs_generator::generate_fb_write(fs_inst *inst,
>>> >> struct brw_reg payload)
>>> >> > if (inst->base_mrf >= 0)
>>> >> > payload = brw_message_reg(inst->base_mrf);
>>> >> >
>>> >> > - /* Header is 2 regs, g0 and g1 are the contents. g0 will be
>>> implied
>>> >> > - * move, here's g1.
>>> >> > - */
>>> >> > - if (inst->header_size != 0) {
>>> >> > - brw_push_insn_state(p);
>>> >> > - brw_set_default_mask_control(p, BRW_MASK_DISABLE);
>>> >> > - brw_set_default_exec_size(p, BRW_EXECUTE_1);
>>> >> > - brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
>>> >> > - brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>>> >> > - brw_set_default_flag_reg(p, 0, 0);
>>> >> > -
>>> >> > - /* On HSW, the GPU will use the predicate on SENDC, unless the
>>> >> header is
>>> >> > - * present.
>>> >> > - */
>>> >> > - if (prog_data->uses_kill) {
>>> >> > - struct brw_reg pixel_mask;
>>> >> > -
>>> >> > - if (devinfo->gen >= 6)
>>> >> > - pixel_mask = retype(brw_vec1_grf(1, 7),
>>> >> BRW_REGISTER_TYPE_UW);
>>> >> > - else
>>> >> > - pixel_mask = retype(brw_vec1_grf(0, 0),
>>> >> BRW_REGISTER_TYPE_UW);
>>> >> > -
>>> >> > - brw_MOV(p, pixel_mask, brw_flag_reg(0, 1));
>>> >> > - }
>>> >> > -
>>> >> > - if (devinfo->gen >= 6) {
>>> >> > - brw_push_insn_state(p);
>>> >> > - brw_set_default_exec_size(p, BRW_EXECUTE_16);
>>> >> > - brw_set_default_compression_control(p,
>>> >> BRW_COMPRESSION_COMPRESSED);
>>> >> > - brw_MOV(p,
>>> >> > - retype(payload, BRW_REGISTER_TYPE_UD),
>>> >> > - retype(brw_vec8_grf(0, 0), BRW_REGISTER_TYPE_UD));
>>> >> > - brw_pop_insn_state(p);
>>> >> > -
>>> >> > - if (inst->target > 0 && key->replicate_alpha) {
>>> >> > - /* Set "Source0 Alpha Present to RenderTarget" bit in
>>> >> message
>>> >> > - * header.
>>> >> > - */
>>> >> > - brw_OR(p,
>>> >> > - vec1(retype(payload, BRW_REGISTER_TYPE_UD)),
>>> >> > - vec1(retype(brw_vec8_grf(0, 0),
>>> BRW_REGISTER_TYPE_UD)),
>>> >> > - brw_imm_ud(0x1 << 11));
>>> >> > - }
>>> >> > -
>>> >> > - if (inst->target > 0) {
>>> >> > - /* Set the render target index for choosing BLEND_STATE. */
>>> >> > - brw_MOV(p, retype(vec1(suboffset(payload, 2)),
>>> >> > - BRW_REGISTER_TYPE_UD),
>>> >> > - brw_imm_ud(inst->target));
>>> >> > - }
>>> >> > -
>>> >> > - /* Set computes stencil to render target */
>>> >> > - if (prog_data->computed_stencil) {
>>> >> > - brw_OR(p,
>>> >> > - vec1(retype(payload, BRW_REGISTER_TYPE_UD)),
>>> >> > - vec1(retype(brw_vec8_grf(0, 0),
>>> >> BRW_REGISTER_TYPE_UD)),
>>> >> > - brw_imm_ud(0x1 << 14));
>>> >> > - }
>>> >> > - }
>>> >> > -
>>> >> > - brw_pop_insn_state(p);
>>> >> > - }
>>> >> > -
>>> >> > if (!runtime_check_aads_emit) {
>>> >> > fire_fb_write(inst, payload, implied_header, inst->mlen);
>>> >> > } else {
>>> >> > --
>>> >> > 2.5.0.400.gff86faf
>>> >> >
>>> >> > _______________________________________________
>>> >> > mesa-dev mailing list
>>> >> > [email protected]
>>> >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>> >>
>>>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
