[Mesa-dev] [PATCH 12/24] i965: Initialize registers' file to BAD_FILE.

[Kenneth Graunke]

On Wednesday, November 11, 2015 03:12:11 PM Matt Turner wrote:
> On Wed, Nov 11, 2015 at 3:05 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > On Wednesday, November 11, 2015 01:07:24 PM Matt Turner wrote:
> >> On Wed, Nov 11, 2015 at 12:46 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> >> > On Monday, November 02, 2015 04:29:22 PM Matt Turner wrote:
> >> >> The test (file == BAD_FILE) works on registers for which the constructor
> >> >> has not run because BAD_FILE is zero.  The next commit will move
> >> >> BAD_FILE in the enum so that it's no longer zero.
> >> >> ---
> >> >>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp     | 10 +++++++++-
> >> >>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |  3 +++
> >> >>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp   |  9 +++++++++
> >> >>  3 files changed, 21 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> >> index 7eeff93..611347c 100644
> >> >> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> >> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> >> @@ -260,6 +260,10 @@ void
> >> >>  fs_visitor::nir_emit_system_values()
> >> >>  {
> >> >>     nir_system_values = ralloc_array(mem_ctx, fs_reg, SYSTEM_VALUE_MAX);
> >> >> +   for (unsigned i = 0; i < SYSTEM_VALUE_MAX; i++) {
> >> >> +      nir_system_values[i].file = BAD_FILE;
> >> >
> >> > How about we do this instead:
> >> >
> >> >    nir_system_values[i] = fs_reg();
> >> >
> >> > That way, they're properly constructed using the default constructor,
> >> > which would not only set BAD_FILE, but properly initialize everything,
> >> > so we don't have to revisit this if we make other changes in fs_reg().
> >>
> >> Is it worth is? The function this code exists in is the thing that
> >> initializes the system values. And, of course if file == BAD_FILE, no
> >> other fields mean anything. Neither of those are likely to change.
> >
> > Yes.  It's the correct thing to do.  We don't want partially
> > initialized objects.  That way lies madness.
> 
> Fine. But I think that calloc()ing storage, memsetting it to zero, and
> then memsetting it to zero again before initializing it is madness.

It's a bit overkill, sure.

The fact that ralloc_array uses calloc internally is an implementation
detail, though - we shouldn't rely on it memsetting the memory to zero.
(rzalloc_array is guaranteed to have those semantics.)

But, putting away ralloc for a moment, you get two memsets even with
ordinary C++.  For example, if you did:

   fs_reg array_of_regs[16];

   for (int i = 0; i < 16; i++)
      array_of_regs[i] = fs_reg(i);

C++ would automatically call the fs_reg() default constructor on all
the fs_regs in the array (1 memset per element).  Then we'd call the
fs_reg constructor when creating fs_reg(i), doing a second memset per
element.

Similarly, using C++'s heap allocator:

   fs_reg array_of_regs[] = new [] fs_reg;

would also call the fs_reg() default constructor for each element,
doing a memset-per-element.

Simply malloc'ing memory for objects and beginning to use them may
work, and even save a bit of code, but it's awful practice in C++.
Constructors need to be called.

Actually, your earlier statement:
"if file == BAD_FILE, no other fields mean anything."
suggests that we should change fs_reg() to simply set BAD_FILE, and
not bother initializing the other fields.  That would eliminate one
of the redundant memsets, and give us valgrind errors if we used
an uninitialized value.  If we do that, though, we should make
equals() return true for two BAD_FILE registers.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151111/dbeb52a0/attachment.sig>