<html><head></head><body><div>On Fri, 2017-10-27 at 12:28 -0700, Jason Ekstrand wrote:</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 27, 2017 at 12:09 AM, Iago Toral <span dir="ltr"><<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>></span> wrote:<br><blockquote type="cite"><div class="gmail-HOEnZb"><div class="gmail-h5"><br>
On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:<br>
> ---<br>
> src/intel/compiler/brw_fs.cpp | 10 ++++++++--<br>
> 1 file changed, 8 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/intel/compiler/brw_fs.<wbr>cpp<br>
> b/src/intel/compiler/brw_fs.<wbr>cpp<br>
> index 1c4351b..52079d3 100644<br>
> --- a/src/intel/compiler/brw_fs.<wbr>cpp<br>
> +++ b/src/intel/compiler/brw_fs.<wbr>cpp<br>
> @@ -2416,8 +2416,14 @@ fs_visitor::opt_algebraic()<br>
> progress = true;<br>
> } else if (inst->src[1].file == IMM) {<br>
> inst->opcode = BRW_OPCODE_MOV;<br>
> - inst->src[0] = component(inst->src[0],<br>
> - <wbr> inst->src[1].ud);<br>
> + /* It's possible that the selected component will be too<br>
> large and<br>
> + * overflow the register. If this happens and we some<br>
> how manage<br>
> + * to constant fold it in and get here, it would cause<br>
> an assert<br>
> + * in component() below. Instead, just let it wrap<br>
> around if it<br>
> + * goes over exec_size.<br>
> + */<br>
<br>
</div></div>component() is really a horiz_offset() call which is in turn a<br>
byte_offset() call, which doesn't assert on anything other than invalid<br>
register files. I guess you mean that the byte offset computed by the<br>
component() call below can later lead to hitting assertions as we<br>
attempt to read outside the allocated space for the vgrf, right?<br></blockquote><div><br></div><div>Yes. Nothing asserts here, we just end up reading way past the end of the VGRF and the validator which checks whether or not we keep all our reads in-bounds throws a fit.<br></div><div> </div><blockquote type="cite">
My question is whether this is supposed to happen at all, it seems like<br>
we should not be emitting broadcast operations like this at all since<br>
they are invalid and here we are instead papering over that bug.<br>
<br></blockquote></div></div><div class="gmail_extra"><br></div><div>This can happen if someone does a read_invocation from SPIR-V or GLSL that has an invocation index that is too large. We're allowed to give them garbage values, but asserting isn't nice.</div></div></blockquote><div><br></div><div>Oh, the bogus value can be directly provided by the user... I don't know if asserting in this case is more or less useful to the user to be honest, I guess the assert message won't be very useful to users anyway.</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">I've updated the comment to:</div><div class="gmail_extra"><br></div><div class="gmail_extra"> /* It's possible that the selected component will be too large and<br> * overflow the register. This can happen if someone does a<br> * readInvocation() from GLSL or SPIR-V and provides an OOB<br> * invocationIndex. If this happens and we some how manage<br> * to constant fold it in and get here, then component() may cause<br> * us to start reading outside of the VGRF which will lead to an<br> * assert later. Instead, just let it wrap around if it goes over<br> * exec_size.<br> */<br></div></div></blockquote><div><br></div><div>Thanks! I think this makes the scenario more clear for future reference.</div><div><br></div><div>Iago</div></body></html>