Re: [Mesa-dev] [PATCH 09/15] i965: Add a new representation for Broadwell shader instructions.

2013-11-18 Thread Eric Anholt 
Kenneth Graunke kenn...@whitecape.org writes:

 Broadwell significantly changes the EU instruction encoding.  Many of
 the fields got moved to different bit positions; some even got split
 in two.

 With so many changes, it was infeasible to continue using struct
 brw_instruction.  We needed a new representation.

 This new approach is a bit different: rather than a struct, I created a
 class that has four DWords, and helper functions that read/write various
 bits.  This has several advantages:

 1. We can create several different names for the same bits.  For
example, conditional modifiers, SFID for SEND instructions, and the
MATH instruction's function opcode are all stored in bits 27:24.

In each situation, we can use the appropriate setter function:
set_sfid(), set_math_function(), or set_cond_modifier().  This
is much easier to follow.

 2. Since the fields are expressed using the original 128-bit numbers,
the code to create the getter/setter functions follows the table in
the documentation very closely.

 To aid in debugging, I've enabled -fkeep-inline-functions when building
 gen8_instruction.cpp.  Otherwise, these functions cannot be called by
 gdb, making it insanely difficult to print out anything.

I dislike C++ creep.  I think the old structs worked OK, and there are
some minor downsides to access-everything-through-methods, like being
unable to just print the instruction in gdb and see fields all at once,
and having to do the build system hack to inline but also keep the
inlines.  But I'm feeling more and more alone on the team.

 diff --git a/src/mesa/drivers/dri/i965/gen8_instruction.cpp 
 b/src/mesa/drivers/dri/i965/gen8_instruction.cpp
 new file mode 100644
 index 000..a6f8667
 --- /dev/null
 +++ b/src/mesa/drivers/dri/i965/gen8_instruction.cpp

 +void
 +gen8_instruction::set_dst(struct brw_reg reg)
 +{
 +   /* MRFs haven't existed since Gen7, so we better not be using them. */
 +   if (reg.file == BRW_MESSAGE_REGISTER_FILE) {
 +  reg.file = BRW_GENERAL_REGISTER_FILE;
 +  reg.nr += GEN7_MRF_HACK_START;
 +   }
 +
 +   assert(reg.file != BRW_MESSAGE_REGISTER_FILE);

The comment doesn't match up with code just below to support it.  And I
think you could just drop the assert if you're going to still do the MRF
hack.  (Same stuff is in set_src0/set_src1).

 +void
 +gen8_instruction::validate_reg(struct brw_reg reg)
 +{
 +   int hstride_for_reg[] = {0, 1, 2, 4};
 +   int vstride_for_reg[] = {0, 1, 2, 4, 8, 16, 32, 64, 128, 256};
 +   int width_for_reg[] = {1, 2, 4, 8, 16};
 +   int execsize_for_reg[] = {1, 2, 4, 8, 16};
 +   int width, hstride, vstride, execsize;
 +
 +   if (reg.file == BRW_IMMEDIATE_VALUE) {
 +  /* TODO(gen8): check immediate vectors */
 +  return;
 +   }
 +
 +   if (reg.file == BRW_ARCHITECTURE_REGISTER_FILE)
 +  return;
 +
 +   assert(reg.hstride = 0  reg.hstride  Elements(hstride_for_reg));
 +   hstride = hstride_for_reg[reg.hstride];
 +
 +   if (reg.vstride == 0xf) {
 +  vstride = -1;
 +   } else {
 +  assert(reg.vstride = 0  reg.vstride  Elements(vstride_for_reg));
 +  vstride = vstride_for_reg[reg.vstride];
 +   }
 +
 +   assert(reg.width = 0  reg.width  Elements(width_for_reg));
 +   width = width_for_reg[reg.width];
 +
 +   assert(exec_size() = 0  exec_size()  Elements(execsize_for_reg));
 +   execsize = execsize_for_reg[exec_size()];
 +
 +   /* Restrictions from 3.3.10: Register Region Restrictions. */
 +   /* 3. */
 +   assert(execsize = width);
 +
 +   /* 4. */
 +   if (execsize == width  hstride != 0) {
 +  assert(vstride == -1 || vstride == width * hstride);
 +   }
 +
 +   /* 5. */
 +   if (execsize == width  hstride == 0) {
 +  /* no restriction on vstride. */
 +   }
 +
 +   /* 6. */
 +   if (width == 1) {
 +  assert(hstride == 0);
 +   }
 +
 +   /* 7. */
 +   if (execsize == 1  width == 1) {
 +  assert(hstride == 0);
 +  assert(vstride == 0);
 +   }
 +
 +   /* 8. */
 +   if (vstride == 0  hstride == 0) {
 +  assert(width == 1);
 +   }

In my spec snapshot, these are 3.3.9.2 A-H now, but there don't seem to
be any changes for bdw.  We should really add checks for those other
region restrictions, though.  And maybe move all this to after the
instruction is all set up, since we do some tweaks of the strides and
things after validate_reg() in set_src0()/1.  Things for later.

 +void
 +gen8_instruction::set_src0(struct brw_reg reg)
 +{

 +   validate_reg(reg);
 +
 +   set_src0_reg_file(reg.file);
 +   set_src0_reg_type(reg.type);
 +   set_src0_abs(reg.abs);
 +   set_src0_negate(reg.negate);
 +
 +   assert(reg.address_mode == BRW_ADDRESS_DIRECT);
 +
 +   if (reg.file == BRW_IMMEDIATE_VALUE) {
 +  data[3] = reg.dw1.ud;
 +
 +  /* Required to set some fields in src1 as well: */
 +  set_src1_reg_file(0); /* arf */

Just use BRW_ARCHITECTURE_REGISTER_FILE?  (I realize this is copy and
paste from brw_eu_emit.c)

 +void
 +gen8_instruction::set_urb_message(enum

Re: [Mesa-dev] [PATCH 09/15] i965: Add a new representation for Broadwell shader instructions.

2013-11-18 Thread Kenneth Graunke 
On 11/18/2013 12:27 PM, Eric Anholt wrote:
 Kenneth Graunke kenn...@whitecape.org writes:
 
 Broadwell significantly changes the EU instruction encoding.  Many of
 the fields got moved to different bit positions; some even got split
 in two.

 With so many changes, it was infeasible to continue using struct
 brw_instruction.  We needed a new representation.

 This new approach is a bit different: rather than a struct, I created a
 class that has four DWords, and helper functions that read/write various
 bits.  This has several advantages:

 1. We can create several different names for the same bits.  For
example, conditional modifiers, SFID for SEND instructions, and the
MATH instruction's function opcode are all stored in bits 27:24.

In each situation, we can use the appropriate setter function:
set_sfid(), set_math_function(), or set_cond_modifier().  This
is much easier to follow.

 2. Since the fields are expressed using the original 128-bit numbers,
the code to create the getter/setter functions follows the table in
the documentation very closely.

 To aid in debugging, I've enabled -fkeep-inline-functions when building
 gen8_instruction.cpp.  Otherwise, these functions cannot be called by
 gdb, making it insanely difficult to print out anything.
 
 I dislike C++ creep.

I wrote this in C++ because all of the compiler infrastructure which
uses it is already in C++.  It seemed natural.

However, Damien requested that we write it in C so it could be easily
copied to xf86-video-intel, intel-gpu-tools, gen4asm/disasm, etc.  Which
is probably a good thing.  He actually already ported it to C.

I can dig up that version and see how you like it...

 I think the old structs worked OK, and there are
 some minor downsides to access-everything-through-methods, like being
 unable to just print the instruction in gdb and see fields all at once,

Huh.  I've never done that.  Is that something you actually do?  I
imagine looking at the raw dump would be pretty huge, and a
pretty-printer/disassembler would always be more useful...

 and having to do the build system hack to inline but also keep the
 inlines.  But I'm feeling more and more alone on the team.

Well, you're less alone than you think.

I believe the build system hack is fairly orthogonal to the C++ issue.
We could also have small inline C functions defined in a .h file which
also take the get_bits/set_bits approach.

I would have appreciated getting I don't like your design feedback
sooner than a year after I wrote it.  But the most important thing is to
have the system we want going forward, which people are happy with and
can maintain.  So I'm open to rewriting it (just a bit grumpy).

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 09/15] i965: Add a new representation for Broadwell shader instructions.

2013-11-18 Thread Kenneth Graunke 
On 11/18/2013 03:22 PM, Kenneth Graunke wrote:
 On 11/18/2013 12:27 PM, Eric Anholt wrote:
[snip]
 I think the old structs worked OK, and there are
 some minor downsides to access-everything-through-methods, like being
 unable to just print the instruction in gdb and see fields all at once,
 
 Huh.  I've never done that.  Is that something you actually do?  I
 imagine looking at the raw dump would be pretty huge, and a
 pretty-printer/disassembler would always be more useful...

In particular,
(gdb) p p-store[0]

prints 114 lines of text in a format that I find basically unreadable.
It also contains information in a redundant form, with sections that
only apply to G45, or Sandybridge, or...

So I'm having a hard time believing this is what you want.  I think a
print function which does something like:

opcode:   0x84
pad:  0
access_mode:  1
mask_control: 0
...

might be more useful...

OTOH,
(gdb) p p-store[0].header

prints a lot less data, and is pretty readable...

Or am I misunderstanding entirely?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 09/15] i965: Add a new representation for Broadwell shader instructions.

2013-11-18 Thread Kenneth Graunke 
On 11/18/2013 03:22 PM, Kenneth Graunke wrote:
 On 11/18/2013 12:27 PM, Eric Anholt wrote:
 Kenneth Graunke kenn...@whitecape.org writes:

 Broadwell significantly changes the EU instruction encoding.  Many of
 the fields got moved to different bit positions; some even got split
 in two.

 With so many changes, it was infeasible to continue using struct
 brw_instruction.  We needed a new representation.

 This new approach is a bit different: rather than a struct, I created a
 class that has four DWords, and helper functions that read/write various
 bits.  This has several advantages:

 1. We can create several different names for the same bits.  For
example, conditional modifiers, SFID for SEND instructions, and the
MATH instruction's function opcode are all stored in bits 27:24.

In each situation, we can use the appropriate setter function:
set_sfid(), set_math_function(), or set_cond_modifier().  This
is much easier to follow.

 2. Since the fields are expressed using the original 128-bit numbers,
the code to create the getter/setter functions follows the table in
the documentation very closely.

 To aid in debugging, I've enabled -fkeep-inline-functions when building
 gen8_instruction.cpp.  Otherwise, these functions cannot be called by
 gdb, making it insanely difficult to print out anything.

 I dislike C++ creep.
 
 I wrote this in C++ because all of the compiler infrastructure which
 uses it is already in C++.  It seemed natural.
 
 However, Damien requested that we write it in C so it could be easily
 copied to xf86-video-intel, intel-gpu-tools, gen4asm/disasm, etc.  Which
 is probably a good thing.  He actually already ported it to C.

For reference, Damien's C port of this code is in the public
intel-gpu-tools repo as assember/gen8_instruction.[ch]:

http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/plain/assembler/gen8_instruction.h
http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/plain/assembler/gen8_instruction.c

So I guess we have some options:
1. Use the C++ version of my bitfield approach.
2. Use Damien's C port of my bitfield approach.
3. Rewrite it as a struct of union of structs again.

Having a system that Eric likes is important to me.  I'm interested to
hear other opinions as well...

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 09/15] i965: Add a new representation for Broadwell shader instructions.

2013-11-12 Thread Kenneth Graunke 
Broadwell significantly changes the EU instruction encoding.  Many of
the fields got moved to different bit positions; some even got split
in two.

With so many changes, it was infeasible to continue using struct
brw_instruction.  We needed a new representation.

This new approach is a bit different: rather than a struct, I created a
class that has four DWords, and helper functions that read/write various
bits.  This has several advantages:

1. We can create several different names for the same bits.  For
   example, conditional modifiers, SFID for SEND instructions, and the
   MATH instruction's function opcode are all stored in bits 27:24.

   In each situation, we can use the appropriate setter function:
   set_sfid(), set_math_function(), or set_cond_modifier().  This
   is much easier to follow.

2. Since the fields are expressed using the original 128-bit numbers,
   the code to create the getter/setter functions follows the table in
   the documentation very closely.

To aid in debugging, I've enabled -fkeep-inline-functions when building
gen8_instruction.cpp.  Otherwise, these functions cannot be called by
gdb, making it insanely difficult to print out anything.

Signed-off-by: Kenneth Graunke kenn...@whitecape.org
---
 src/mesa/drivers/dri/i965/Makefile.am  |   2 +
 src/mesa/drivers/dri/i965/Makefile.sources |   1 +
 src/mesa/drivers/dri/i965/gen8_instruction.cpp | 353 +
 src/mesa/drivers/dri/i965/gen8_instruction.h   | 262 ++
 4 files changed, 618 insertions(+)
 create mode 100644 src/mesa/drivers/dri/i965/gen8_instruction.cpp
 create mode 100644 src/mesa/drivers/dri/i965/gen8_instruction.h

diff --git a/src/mesa/drivers/dri/i965/Makefile.am 
b/src/mesa/drivers/dri/i965/Makefile.am
index 8c0f9a3..645901f 100644
--- a/src/mesa/drivers/dri/i965/Makefile.am
+++ b/src/mesa/drivers/dri/i965/Makefile.am
@@ -38,6 +38,8 @@ AM_CFLAGS = \
 
 AM_CXXFLAGS = $(AM_CFLAGS)
 
+gen8_instruction_CXXFLAGS = $(AM_CXXFLAGS) -fkeep-inline-functions
+
 noinst_LTLIBRARIES = libi965_dri.la
 libi965_dri_la_SOURCES = $(i965_FILES)
 libi965_dri_la_LIBADD = $(INTEL_LIBS)
diff --git a/src/mesa/drivers/dri/i965/Makefile.sources 
b/src/mesa/drivers/dri/i965/Makefile.sources
index 4714100..df34ef3 100644
--- a/src/mesa/drivers/dri/i965/Makefile.sources
+++ b/src/mesa/drivers/dri/i965/Makefile.sources
@@ -135,4 +135,5 @@ i965_FILES = \
gen7_vs_state.c \
gen7_wm_state.c \
gen7_wm_surface_state.c \
+   gen8_instruction.cpp \
 $()
diff --git a/src/mesa/drivers/dri/i965/gen8_instruction.cpp 
b/src/mesa/drivers/dri/i965/gen8_instruction.cpp
new file mode 100644
index 000..a6f8667
--- /dev/null
+++ b/src/mesa/drivers/dri/i965/gen8_instruction.cpp
@@ -0,0 +1,353 @@
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+/** @file gen8_instruction.cpp
+ *
+ * A representation of a Gen8+ EU instruction, with helper methods to get
+ * and set various fields.  This is the actual hardware format.
+ */
+
+#include brw_defines.h
+#include gen8_instruction.h
+
+void
+gen8_instruction::set_dst(struct brw_reg reg)
+{
+   /* MRFs haven't existed since Gen7, so we better not be using them. */
+   if (reg.file == BRW_MESSAGE_REGISTER_FILE) {
+  reg.file = BRW_GENERAL_REGISTER_FILE;
+  reg.nr += GEN7_MRF_HACK_START;
+   }
+
+   assert(reg.file != BRW_MESSAGE_REGISTER_FILE);
+
+   if (reg.file == BRW_GENERAL_REGISTER_FILE)
+  assert(reg.nr  BRW_MAX_GRF);
+
+   set_dst_reg_file(reg.file);
+   set_dst_reg_type(reg.type);
+
+   assert(reg.address_mode == BRW_ADDRESS_DIRECT);
+
+   set_dst_da_reg_nr(reg.nr);
+
+   if (access_mode() == BRW_ALIGN_1) {
+  /* Set Dst.SubRegNum[4:0] */
+  set_dst_da1_subreg_nr(reg.subnr);
+
+  /* Set Dst.HorzStride */
+  if (reg.hstride == BRW_HORIZONTAL_STRIDE_0)
+ reg.hstride =