<div dir="ltr">On 2 December 2013 11:31, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra"><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h<br>
index ff5af93..f284389 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_shader.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_shader.h<br>
@@ -23,6 +23,7 @@<br>
<br>
#include <stdint.h><br>
#include "brw_defines.h"<br>
+#include "brw_reg.h"<br>
#include "glsl/ir.h"<br>
<br>
#pragma once<br>
@@ -39,6 +40,45 @@ enum register_file {<br>
<br>
#ifdef __cplusplus<br>
<br>
+class backend_reg {<br>
+public:<br>
+ backend_reg();<br>
+ backend_reg(struct brw_reg reg);<br>
+<br>
+ bool is_zero() const;<br>
+ bool is_one() const;<br>
+ bool is_null() const;<br>
+<br>
+ /** Register file: GRF, MRF, IMM. */<br>
+ enum register_file file;<br>
+<br>
+ /**<br>
+ * Register number. For MRF, it's the hardware register. For<br>
+ * GRF, it's a virtual register number until register allocation<br>
+ */<br>
+ int reg;<br>
+<br>
+ /**<br>
+ * Offset from the start of the contiguous register block.<br>
+ *<br>
+ * For pre-register-allocation GRFs, this is in units of a float per pixel<br>
+ * (1 hardware register for SIMD8 mode, or 2 registers for SIMD16 mode).<br></blockquote></div><div class="gmail_quote"><div><br></div><div>Since this code is now shared with the vec4 back-end, can we update this comment to say "1 hardware register for SIMD8 or SIMD4x2 mode..."?<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">
+ * For uniforms, this is in units of 1 float.<br>
+ */<br>
+ int reg_offset;<br>
+<br>
+ /** Register type. BRW_REGISTER_TYPE_* */<br>
+ int type;<br>
+ struct brw_reg fixed_hw_reg;<br>
+<br>
+ /** Value for file == BRW_IMMMEDIATE_FILE */<br>
+ union {<br>
+ int32_t i;<br>
+ uint32_t u;<br>
+ float f;<br>
+ } imm;<br>
+};<br>
+<br>
class backend_instruction : public exec_node {<br>
public:<br>
bool is_tex();<br>
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
index 3b3f35b..a718333 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_vec4.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
@@ -89,28 +89,7 @@ class dst_reg;<br>
unsigned<br>
swizzle_for_size(int size);<br>
<br>
-class reg<br>
-{<br>
-public:<br>
- /** Register file: GRF, MRF, IMM. */<br>
- enum register_file file;<br>
- /** virtual register number. 0 = fixed hw reg */<br>
- int reg;<br>
- /** Offset within the virtual register. */<br>
- int reg_offset;<br>
- /** Register type. BRW_REGISTER_TYPE_* */<br>
- int type;<br>
- struct brw_reg fixed_hw_reg;<br>
-<br>
- /** Value for file == BRW_IMMMEDIATE_FILE */<br>
- union {<br>
- int32_t i;<br>
- uint32_t u;<br>
- float f;<br>
- } imm;<br>
-};<br>
-<br>
-class src_reg : public reg<br>
+class src_reg : public backend_reg<br>
{<br>
public:<br>
DECLARE_RALLOC_CXX_OPERATORS(src_reg)<br>
@@ -123,10 +102,9 @@ public:<br>
src_reg(uint32_t u);<br>
src_reg(int32_t i);<br>
src_reg(struct brw_reg reg);<br>
+ src_reg(const backend_reg ®);<br></blockquote><div><br></div><div><div>I'm concerned about this constructor (and
the corresponding constructors in the dst_reg and fs_reg classes) contributing to object slicing problems. Consider this code:<br><br></div><div> src_reg r1;<br></div><div> r1.swizzle = BRW_SWIZZLE_XXXX;<br></div>
<div> backend_reg r2(r1);<br></div><div> src_reg r3(r2);<br></div><div>
assert(r3.swizzle == BRW_SWIZZLE_XXXX); /* fails! */<br><br></div><div>The
reason for the failure is that src_reg::swizzle doesn't appear in the
backend_reg base class. So when r1 is copied to r2, the value of swizzle is lost. That by itself wouldn't be a problem, except that when we later try to reconstruct a src_reg from r2, swizzle winds up being initialized to the default value.<br>
<br></div><div>Of course we would never write code like the above example directly, but since your shared code for implementing loads/stores (in future patches) uses backend_reg
for all of its register representations, it seems likely that a src_reg
will get converted to a backend_reg and then back to a src_reg at some
point during compilation. So I suspect that when compiling GLSL source
code like the following:<br></div><br><div class="gmail_quote"> uniform image2D img;<br></div><div class="gmail_quote"> ivec2 coord;<br></div><div class="gmail_quote"> vec4 v;<br></div><div> imageStore(img, coord, v.yzwx);<br>
<br></div><div>The swizzle would fail to take effect.<br><br>Have you run tests to see if this is in fact the case?<br></div><div><br></div><div>Unfortunately I don't really have a good suggestion as to what to do about this. I'll continue reviewing the series assuming this problem isn't present, but I think we need to figure out a solution before we can land the series.<br>
</div></div></div></div></div>