<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 14, 2019 at 4:53 PM Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes: > On Fri, Jan 18, 2019 at 6:09 PM Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> > wrote: > >> This is required in combination with the following commit, because >> otherwise if a source region with an extended 8+ stride is present in >> the instruction (which we're about to declare legal) we'll end up >> emitting code that attempts to write to such a region, even though >> strides greater than four are still illegal for the destination. >> --- >> src/intel/compiler/brw_fs_lower_regioning.cpp | 20 ++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/src/intel/compiler/brw_fs_lower_regioning.cpp >> b/src/intel/compiler/brw_fs_lower_regioning.cpp >> index 6a3c23892b4..b86e95ed9eb 100644 >> --- a/src/intel/compiler/brw_fs_lower_regioning.cpp >> +++ b/src/intel/compiler/brw_fs_lower_regioning.cpp >> @@ -71,15 +71,25 @@ namespace { >> !is_byte_raw_mov(inst)) { >> return get_exec_type_size(inst); >> } else { >> - unsigned stride = inst->dst.stride * type_sz(inst->dst.type); >> + /* Calculate the maximum byte stride and the minimum type size >> across >> + * all source and destination operands. >> + */ >> + unsigned max_stride = inst->dst.stride * type_sz(inst->dst.type); >> + unsigned min_size = type_sz(inst->dst.type); >> >> for (unsigned i = 0; i < inst->sources; i++) { >> - if (!is_uniform(inst->src[i]) && !inst->is_control_source(i)) >> - stride = MAX2(stride, inst->src[i].stride * >> - type_sz(inst->src[i].type)); >> + if (!is_uniform(inst->src[i]) && !inst->is_control_source(i)) >> { >> + max_stride = MAX2(max_stride, inst->src[i].stride * >> + type_sz(inst->src[i].type)); >> + min_size = MIN2(min_size, type_sz(inst->src[i].type)); >> + } >> } >> >> - return stride; >> + /* Attempt to use the largest byte stride among all present >> operands, >> + * but never exceed a stride of 4 since that would lead to >> illegal >> + * destination regions during lowering. >> + */ >> + return MIN2(max_stride, 4 * min_size); >> > > Why not just fall back to tightly packed in this case? I think I can > answer my own question: Because using something that's equal to one of the > strides reduces the liklihood that we'll need a temporary. If that's the > correct answer, then maybe what we want is the maximum of all strides with > stride_in_bytes <= 4 * type_sz? > We also want the result to be greater than or equal to the size of the largest non-uniform, non-control source type, since packing a vector of such a type into a temporary of lower byte stride than its size is impossible. This patch guarantees that as long as max_size <= 4 * min_size, which is necessary for the lowering code that calls this function to work at all. It would be possible to preserve this guarantee while attempting to pick one of the strides of the pre-existing sources as you say -- I would be happy to review that change as a follow-up micro-optimization patch, but there are some corner cases to consider I don't necessarily want to bother with in the patch doing the functional change, for the sake of bisectability. </blockquote><div> </div><div>That's fine with me.</div><div> </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </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"> It may make sense to add an assert here that max_size <= 4 * min_size for the case such an instruction doesn't blow up already at the EU validator (it doesn't look like the validator is currently enforcing the lack of conversions between 8 and 64 bit types?), it will just involve calculating max_size in addition to max_stride and min_size above. > --Jason > > >> } >> } >> >> -- >> 2.19.2 >> >> _______________________________________________ >> mesa-dev mailing list >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> </blockquote></div></div>