<div dir="ltr"><div><div>Looks good to me.  Thanks for catching this!<br><br></div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div>Fixes: 1e5b09f42f694687ac "spirv: Tidy some repeated if checks..."<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 2, 2018 at 10:49 AM, Neil Roberts <span dir="ltr"><<a href="mailto:nroberts@igalia.com" target="_blank">nroberts@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This behaviour was changed in 1e5b09f42f694687ac. The commit message<br>
for that says it is just a “tidy up” so my assumption is that the<br>
behaviour change was a mistake. It’s a little hard to decipher looking<br>
at the diff, but the previous code before that patch was:<br>
<br>
  if (builtin == SpvBuiltInFragCoord || builtin == SpvBuiltInSamplePosition)<br>
     nir_var->data.origin_upper_<wbr>left = b->origin_upper_left;<br>
<br>
  if (builtin == SpvBuiltInFragCoord)<br>
     nir_var->data.pixel_center_<wbr>integer = b->pixel_center_integer;<br>
<br>
After the patch the code was:<br>
<br>
  case SpvBuiltInSamplePosition:<br>
     nir_var->data.origin_upper_<wbr>left = b->origin_upper_left;<br>
     /* fallthrough */<br>
  case SpvBuiltInFragCoord:<br>
     nir_var->data.pixel_center_<wbr>integer = b->pixel_center_integer;<br>
     break;<br>
<br>
Before the patch origin_upper_left affected both builtins and<br>
pixel_center_integer only affected FragCoord. After the patch<br>
origin_upper_left only affects SamplePosition and pixel_center_integer<br>
affects both variables.<br>
<br>
This patch tries to restore the previous behaviour by changing the<br>
code to:<br>
<br>
  case SpvBuiltInFragCoord:<br>
     nir_var->data.pixel_center_<wbr>integer = b->pixel_center_integer;<br>
     /* fallthrough */<br>
  case SpvBuiltInSamplePosition:<br>
     nir_var->data.origin_upper_<wbr>left = b->origin_upper_left;<br>
     break;<br>
<br>
This change will be important for ARB_gl_spirv which is meant to<br>
support OriginLowerLeft.<br>
---<br>
 src/compiler/spirv/vtn_<wbr>variables.c | 6 +++---<br>
 1 file changed, 3 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/compiler/spirv/vtn_<wbr>variables.c b/src/compiler/spirv/vtn_<wbr>variables.c<br>
index 9679ff6526c..fd8ab7f247a 100644<br>
--- a/src/compiler/spirv/vtn_<wbr>variables.c<br>
+++ b/src/compiler/spirv/vtn_<wbr>variables.c<br>
@@ -1419,11 +1419,11 @@ apply_var_decoration(struct vtn_builder *b, nir_variable *nir_var,<br>
       case SpvBuiltInTessLevelInner:<br>
          nir_var->data.compact = true;<br>
          break;<br>
-      case SpvBuiltInSamplePosition:<br>
-         nir_var->data.origin_upper_<wbr>left = b->origin_upper_left;<br>
-         /* fallthrough */<br>
       case SpvBuiltInFragCoord:<br>
          nir_var->data.pixel_center_<wbr>integer = b->pixel_center_integer;<br>
+         /* fallthrough */<br>
+      case SpvBuiltInSamplePosition:<br>
+         nir_var->data.origin_upper_<wbr>left = b->origin_upper_left;<br>
          break;<br>
       default:<br>
          break;<br>
<span class="HOEnZb"><font color="#888888">-- <br>
2.14.3<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div>