<div dir="ltr">On 18 October 2013 17:04, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> writes:<br>
<br>
> Geometry shaders that run in "DUAL_INSTANCED" mode store their inputs<br>
> in vec4's. This means that when compiling gl_PointSize input<br>
> swizzling (a MOV instruction which uses a geometry shader input as<br>
> both source and destination), we need to do two things:<br>
><br>
> - Set force_writemask_all to ensure that the MOV happens regardless of<br>
> which channels are enabled.<br>
><br>
> - Set the source register region to <4;4,1> (instead of <0;4,1> to<br>
> satisfy register region restrictions.<br>
<br>
</div>This sure sounds like something you empirically found, but I'm confused.<br>
I would have assumed that DUAL_INSTANCED with an instance count of 1<br>
meant that the channel enables the hardware gave you had the first 4<br>
enabled and the second 4 disabled. And since the dst.width (and thus<br>
execsize) is 4, whether or not the second 4 are disabled wouldn't<br>
matter. In that case, why do you need the writemask forced, since just<br>
the 4 channels you care about will be affected, anyway?<br></blockquote><div><br></div><div>Setting force_writemask_all was not empirical--I did it out of general caution. I couldn't find any documentation in the bspec to guarantee that an instance count of 1 would get dispatched to the first 4 channels rather than the second 4, and I didn't want to rely on undocumented behaviour.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
And, if dst.width == 4, then execsize == 4, and I'm confused what<br>
register region restriction is being honored by promoting the hstride to<br>
4.<br></blockquote><div><br></div><div>This part was empirical. I discovered that if I don't set the hstride to 4, then I get an assertion failure here (in validate_reg() in brw_eu_emit.c):<br><br> /* 4. */<br> if (execsize == width && hstride != 0) {<br>
assert(vstride == -1 || vstride == width * hstride);<br> }<br><br></div><div>I don't know why the hardware cares about this, but I double-checked the bspec and this restriction is really there.<br><br></div><div>
Side note: I forgot to mention it in the comments, but in addition to fixing up <0;4,1> -> <4;4,1>, this code fixes up <8;8,1> -> <4;4,1>. That's necessary for a stupid reason: in the vec4 back-end we represent GRFs as <8;8,1> so that guess_execution_size() will correctly guess an execution size of 8. However, in align16 mode, the hardware assumes a width of 4, so it really needs to be <4;4,1>. Normally, we fix that up in brw_set_src0() and brw_set_src1(), but we do it *after* the call to validate_reg(). So to avoid validate_reg() asserting on these width-4 instructions, we need to change the source register from <8;8,1> to <4;4,1> before emitting the instruction. One of these days, I swear I'm going to get rid of guess_execution_size() so we can end this sort of madness.<br>
</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Putting these fixups for a couple of weird cases in just MOV and ADD<br>
feels wrong to me, but maybe when I understand better what's going on<br>
it'll seem more natural.<br></blockquote><div><br></div><div>Another possibility I'd be equally happy with would be to put the fixup at the top of vec4_generator::generate_vec4_instruction(), before the switch
statement. It would look something like this:<br><br> if (dst.width == BRW_WIDTH_4) {<br> /* This happens in attribute fixups for "dual instanced" geometry<br> * shaders, since they use attributes that are vec4's. Since the exec<br>
* width is only 4, it's essential that the caller set<br> * force_writemask_all in order to make sure the instruction is executed<br> * regardless of which channels are enabled.<br> */<br> assert(inst->force_writemask_all);<br>
<br> /* Fix up any <8;8,1> or <0;4,1> source registers to <4;4,1> to satisfy<br> * the following register region restrictions (from Graphics BSpec:<br> * 3D-Media-GPGPU Engine > EU Overview > Registers and Register Regions<br>
* > Register Region Restrictions)<br> *<br> * 1. ExecSize must be greater than or equal to Width.<br> *<br> * 2. If ExecSize = Width and HorzStride != 0, VertStride must be set<br>
* to Width * HorzStride."<br> */<br> for (int i = 0; i < 3; i++) {<br> if (src[i].file == BRW_GENERAL_REGISTER_FILE)<br> src[i] = stride(src[i], 4, 4, 1);<br> }<br>
}<br><br></div><div>Does that seem better to you? I actually think I like it slightly better because by making the assertion more general, I caught another case where I think I should be setting force_writemask_all to be on the safe side (the "clear r0.2" instruction in the gs prolog).<br>
</div></div></div></div>