Re: [Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)

2014-11-19 Thread Kenneth Graunke 
On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
 Add support for decoding the new branch control bit. I saw two things wrong 
 with
 the existing code.
 
 1. It didn't bother trying to decode the bit.
 -  While we do not *intentionally* emit this bit today, I think it's 
 interesting
to see if we somehow ended up with the bit set. It may also be useful in 
 the
future.
 
 2. It seemed to be the wrong bit.
 -  The docs are pretty poor wrt which bit this actually occupies. To me, it
/looks/ like it should be bit 28. I am not sure where Ken got 30 from. I
verified it should be 28 by looking at the simulator code.

Yeah, it sure looks like 28 to me...I must've botched it when typing up the
tables.

One comment below.

 I also added the most basic support for GOTO simply so we don't need to 
 remember
 to change the function in the future.
 
 Cc: Kenneth Graunke kenn...@whitecape.org
 Signed-off-by: Ben Widawsky b...@bwidawsk.net
 ---
  src/mesa/drivers/dri/i965/brw_defines.h |  1 +
  src/mesa/drivers/dri/i965/brw_disasm.c  | 29 ++---
  src/mesa/drivers/dri/i965/brw_inst.h|  2 +-
  3 files changed, 28 insertions(+), 4 deletions(-)
 
 diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
 b/src/mesa/drivers/dri/i965/brw_defines.h
 index 53cd75e..ed94bcc 100644
 --- a/src/mesa/drivers/dri/i965/brw_defines.h
 +++ b/src/mesa/drivers/dri/i965/brw_defines.h
 @@ -820,6 +820,7 @@ enum opcode {
 BRW_OPCODE_MSAVE =44,  /** Pre-Gen6 */
 BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
 BRW_OPCODE_PUSH = 46,  /** Pre-Gen6 */
 +   BRW_OPCODE_GOTO = 46,  /** Gen8+*/
 BRW_OPCODE_POP =  47,  /** Pre-Gen6 */
 BRW_OPCODE_WAIT = 48,
 BRW_OPCODE_SEND = 49,
 diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c 
 b/src/mesa/drivers/dri/i965/brw_disasm.c
 index 53ec767..013058e 100644
 --- a/src/mesa/drivers/dri/i965/brw_disasm.c
 +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
 @@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
  }
  
  static bool
 +has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
 +{
 +   if (brw-gen  8)
 +  return false;
 +
 +   return opcode == BRW_OPCODE_IF ||
 +  opcode == BRW_OPCODE_ELSE ||
 +  opcode == BRW_OPCODE_GOTO ||
 +  opcode == BRW_OPCODE_ENDIF;
 +}

I don't think ENDIF has BranchCtrl.  With that removed, this is:

Reviewed-by: Kenneth Graunke kenn...@whitecape.org

signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)

2014-11-19 Thread Ben Widawsky 
On Wed, Nov 19, 2014 at 01:28:25AM -0800, Kenneth Graunke wrote:
 On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
  Add support for decoding the new branch control bit. I saw two things wrong 
  with
  the existing code.
  
  1. It didn't bother trying to decode the bit.
  -  While we do not *intentionally* emit this bit today, I think it's 
  interesting
 to see if we somehow ended up with the bit set. It may also be useful in 
  the
 future.
  
  2. It seemed to be the wrong bit.
  -  The docs are pretty poor wrt which bit this actually occupies. To me, it
 /looks/ like it should be bit 28. I am not sure where Ken got 30 from. I
 verified it should be 28 by looking at the simulator code.
 
 Yeah, it sure looks like 28 to me...I must've botched it when typing up the
 tables.
 
 One comment below.
 
  I also added the most basic support for GOTO simply so we don't need to 
  remember
  to change the function in the future.
  
  Cc: Kenneth Graunke kenn...@whitecape.org
  Signed-off-by: Ben Widawsky b...@bwidawsk.net
  ---
   src/mesa/drivers/dri/i965/brw_defines.h |  1 +
   src/mesa/drivers/dri/i965/brw_disasm.c  | 29 ++---
   src/mesa/drivers/dri/i965/brw_inst.h|  2 +-
   3 files changed, 28 insertions(+), 4 deletions(-)
  
  diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
  b/src/mesa/drivers/dri/i965/brw_defines.h
  index 53cd75e..ed94bcc 100644
  --- a/src/mesa/drivers/dri/i965/brw_defines.h
  +++ b/src/mesa/drivers/dri/i965/brw_defines.h
  @@ -820,6 +820,7 @@ enum opcode {
  BRW_OPCODE_MSAVE =  44,  /** Pre-Gen6 */
  BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
  BRW_OPCODE_PUSH =   46,  /** Pre-Gen6 */
  +   BRW_OPCODE_GOTO =   46,  /** Gen8+*/
  BRW_OPCODE_POP =47,  /** Pre-Gen6 */
  BRW_OPCODE_WAIT =   48,
  BRW_OPCODE_SEND =   49,
  diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c 
  b/src/mesa/drivers/dri/i965/brw_disasm.c
  index 53ec767..013058e 100644
  --- a/src/mesa/drivers/dri/i965/brw_disasm.c
  +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
  @@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
   }
   
   static bool
  +has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
  +{
  +   if (brw-gen  8)
  +  return false;
  +
  +   return opcode == BRW_OPCODE_IF ||
  +  opcode == BRW_OPCODE_ELSE ||
  +  opcode == BRW_OPCODE_GOTO ||
  +  opcode == BRW_OPCODE_ENDIF;
  +}
 
 I don't think ENDIF has BranchCtrl.  With that removed, this is:
 
 Reviewed-by: Kenneth Graunke kenn...@whitecape.org

Yes. Matt caught this as well. Unfortunately the search for branch_ctrl returns
nothing in bspec, so I had to manually click every instruction in the ISA (I
didn't want to assume anything). Else is next to Endif... I am pretty sure I
clicked Else twice.

Thanks. I'll consider the other comment Matt left and then either resubmit with
a change for him, or push.

-- 
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)

2014-11-19 Thread Kenneth Graunke 
On Wednesday, November 19, 2014 09:15:32 AM Ben Widawsky wrote:
 On Wed, Nov 19, 2014 at 01:28:25AM -0800, Kenneth Graunke wrote:
  On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
   Add support for decoding the new branch control bit. I saw two things 
   wrong with
   the existing code.
   
   1. It didn't bother trying to decode the bit.
   -  While we do not *intentionally* emit this bit today, I think it's 
   interesting
  to see if we somehow ended up with the bit set. It may also be useful 
   in the
  future.
   
   2. It seemed to be the wrong bit.
   -  The docs are pretty poor wrt which bit this actually occupies. To me, 
   it
  /looks/ like it should be bit 28. I am not sure where Ken got 30 from. 
   I
  verified it should be 28 by looking at the simulator code.
  
  Yeah, it sure looks like 28 to me...I must've botched it when typing up the
  tables.
  
  One comment below.
  
   I also added the most basic support for GOTO simply so we don't need to 
   remember
   to change the function in the future.
   
   Cc: Kenneth Graunke kenn...@whitecape.org
   Signed-off-by: Ben Widawsky b...@bwidawsk.net
   ---
src/mesa/drivers/dri/i965/brw_defines.h |  1 +
src/mesa/drivers/dri/i965/brw_disasm.c  | 29 
   ++---
src/mesa/drivers/dri/i965/brw_inst.h|  2 +-
3 files changed, 28 insertions(+), 4 deletions(-)
   
   diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
   b/src/mesa/drivers/dri/i965/brw_defines.h
   index 53cd75e..ed94bcc 100644
   --- a/src/mesa/drivers/dri/i965/brw_defines.h
   +++ b/src/mesa/drivers/dri/i965/brw_defines.h
   @@ -820,6 +820,7 @@ enum opcode {
   BRW_OPCODE_MSAVE =44,  /** Pre-Gen6 */
   BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
   BRW_OPCODE_PUSH = 46,  /** Pre-Gen6 */
   +   BRW_OPCODE_GOTO = 46,  /** Gen8+*/
   BRW_OPCODE_POP =  47,  /** Pre-Gen6 */
   BRW_OPCODE_WAIT = 48,
   BRW_OPCODE_SEND = 49,
   diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c 
   b/src/mesa/drivers/dri/i965/brw_disasm.c
   index 53ec767..013058e 100644
   --- a/src/mesa/drivers/dri/i965/brw_disasm.c
   +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
   @@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
}

static bool
   +has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
   +{
   +   if (brw-gen  8)
   +  return false;
   +
   +   return opcode == BRW_OPCODE_IF ||
   +  opcode == BRW_OPCODE_ELSE ||
   +  opcode == BRW_OPCODE_GOTO ||
   +  opcode == BRW_OPCODE_ENDIF;
   +}
  
  I don't think ENDIF has BranchCtrl.  With that removed, this is:
  
  Reviewed-by: Kenneth Graunke kenn...@whitecape.org
 
 Yes. Matt caught this as well. Unfortunately the search for branch_ctrl 
 returns
 nothing in bspec, so I had to manually click every instruction in the ISA (I
 didn't want to assume anything). Else is next to Endif... I am pretty sure I
 clicked Else twice.

Odd, search for BranchCtrl and branch_ctrl turned up things for me.  Just had
to consider the spellings :(

 
 Thanks. I'll consider the other comment Matt left and then either resubmit 
 with
 a change for him, or push.

Oh, I hadn't seen Matt's reply.  I like his suggestion as well.

I don't think it's worth resubmitting - if you make both of those changes,
you can probably just go ahead and push the patch.

signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)

2014-11-18 Thread Ben Widawsky 
Add support for decoding the new branch control bit. I saw two things wrong with
the existing code.

1. It didn't bother trying to decode the bit.
-  While we do not *intentionally* emit this bit today, I think it's interesting
   to see if we somehow ended up with the bit set. It may also be useful in the
   future.

2. It seemed to be the wrong bit.
-  The docs are pretty poor wrt which bit this actually occupies. To me, it
   /looks/ like it should be bit 28. I am not sure where Ken got 30 from. I
   verified it should be 28 by looking at the simulator code.

I also added the most basic support for GOTO simply so we don't need to remember
to change the function in the future.

Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
 src/mesa/drivers/dri/i965/brw_defines.h |  1 +
 src/mesa/drivers/dri/i965/brw_disasm.c  | 29 ++---
 src/mesa/drivers/dri/i965/brw_inst.h|  2 +-
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
b/src/mesa/drivers/dri/i965/brw_defines.h
index 53cd75e..ed94bcc 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -820,6 +820,7 @@ enum opcode {
BRW_OPCODE_MSAVE =  44,  /** Pre-Gen6 */
BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
BRW_OPCODE_PUSH =   46,  /** Pre-Gen6 */
+   BRW_OPCODE_GOTO =   46,  /** Gen8+*/
BRW_OPCODE_POP =47,  /** Pre-Gen6 */
BRW_OPCODE_WAIT =   48,
BRW_OPCODE_SEND =   49,
diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c 
b/src/mesa/drivers/dri/i965/brw_disasm.c
index 53ec767..013058e 100644
--- a/src/mesa/drivers/dri/i965/brw_disasm.c
+++ b/src/mesa/drivers/dri/i965/brw_disasm.c
@@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
 }
 
 static bool
+has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
+{
+   if (brw-gen  8)
+  return false;
+
+   return opcode == BRW_OPCODE_IF ||
+  opcode == BRW_OPCODE_ELSE ||
+  opcode == BRW_OPCODE_GOTO ||
+  opcode == BRW_OPCODE_ENDIF;
+}
+
+static bool
 is_logic_instruction(unsigned opcode)
 {
return opcode == BRW_OPCODE_AND ||
@@ -217,6 +229,11 @@ static const char *const accwr[2] = {
[1] = AccWrEnable
 };
 
+static const char *const branch_ctrl[2] = {
+   [0] = ,
+   [1] = BranchCtrl
+};
+
 static const char *const wectrl[2] = {
[0] = ,
[1] = WE_all
@@ -1544,9 +1561,15 @@ brw_disassemble_inst(FILE *file, struct brw_context 
*brw, brw_inst *inst,
   err |= control(file, compaction, cmpt_ctrl, is_compacted, space);
   err |= control(file, thread control, thread_ctrl,
  brw_inst_thread_control(brw, inst), space);
-  if (brw-gen = 6)
- err |= control(file, acc write control, accwr,
-brw_inst_acc_wr_control(brw, inst), space);
+  if (brw-gen = 6) {
+ if (has_branch_ctrl(brw, opcode)) {
+err |= control(file, branch ctrl, branch_ctrl,
+   brw_inst_branch_control(brw, inst), space);
+ } else {
+err |= control(file, acc write control, accwr,
+   brw_inst_acc_wr_control(brw, inst), space);
+ }
+  }
   if (opcode == BRW_OPCODE_SEND || opcode == BRW_OPCODE_SENDC)
  err |= control(file, end of thread, end_of_thread,
 brw_inst_eot(brw, inst), space);
diff --git a/src/mesa/drivers/dri/i965/brw_inst.h 
b/src/mesa/drivers/dri/i965/brw_inst.h
index c1ff10d..372aa2b 100644
--- a/src/mesa/drivers/dri/i965/brw_inst.h
+++ b/src/mesa/drivers/dri/i965/brw_inst.h
@@ -169,9 +169,9 @@ FF(flag_reg_nr,
/* 8: */ 33, 33)
 F8(flag_subreg_nr, /* 4+ */  89, 89, /* 8+ */ 32, 32)
 F(saturate, 31,  31)
-FC(branch_control,  30,  30, brw-gen = 8)
 F(debug_control,30,  30)
 F(cmpt_control, 29,  29)
+FC(branch_control,  28,  28, brw-gen = 8)
 F(acc_wr_control,   28,  28)
 F(cond_modifier,27,  24)
 FC(math_function,   27,  24, brw-gen = 6)
-- 
2.1.3

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