[hsa-branch] Cleanups of hsa_insns_signal and hsa_insns_queue

Martin Jambor mjambor@suse.cz
Wed Aug 3 14:36:00 GMT 2016


Hi,

currently, hsa_insn_signal is a descendant of hsa_insn_atomic, even
though its BRIG representation actually contains fewer fields than the
atomic BRIG representation so if anything, it should be the other way
around.  But this patch actually makes both direct descendants of
hsa_insn_basic because apart from construction, which needs to
differentiate between them anyway, there is no need for common
processing.

This patch also adds fields to hsa_insn_queue which we have so far
assumed to have one particular value in order to enable more flexibility
in subsequent patches on the hsa branch (Unlike those, I intend to
commit this one to trunk soon as well).

Thanks,

Martin

2016-07-18  Martin Jambor  <mjambor@suse.cz>

	* hsa.h (hsa_insn_signal): Make a direct descendant of
	hsa_insn_basic.  Add memorder constructor parameter and
	m_memory_order and m_signalop member variables.
	(hsa_insn_queue): Changed constructor parameters to common form.
	Added m_segment and m_memory_order member variables.
	* hsa-brig.c (emit_signal_insn): Remove obsolete comment.  Update
	member variable name, pick a type according to profile.
	(emit_alloca_insn): Remove obsolete comment.
	(emit_atomic_insn): Likewise.
	(emit_queue_insn): Get segment and memory order from the IR object.
	* hsa-dump.c (dump_hsa_insn_1): Update signal member variable
	name.  Special dumping for queue objects.
	* hsa-gen.c (hsa_insn_atomic): Fix function comment.
	(hsa_insn_signal::hsa_insn_signal): Fix comment.  Update call to
	ancestor constructor and initialization of new member variables.
	(hsa_insn_queue::hsa_insn_queue): Added initialization of new
	member variables.
	(gen_hsa_insns_for_kernel_call): Use constructor arguments to
	initialize signal memory order, remove signal, memory scope
	initialization.  Use new constructor arguments to initialize queue
	members.  Remove atomic instruction scope initialization.
---
 gcc/hsa-brig.c | 22 +++++---------------
 gcc/hsa-dump.c | 15 +++++++++++---
 gcc/hsa-gen.c  | 65 ++++++++++++++++++++++++++--------------------------------
 gcc/hsa.h      | 28 ++++++++++++++++++++-----
 4 files changed, 69 insertions(+), 61 deletions(-)

diff --git a/gcc/hsa-brig.c b/gcc/hsa-brig.c
index 716d8f5..1edd126 100644
--- a/gcc/hsa-brig.c
+++ b/gcc/hsa-brig.c
@@ -1333,10 +1333,6 @@ emit_signal_insn (hsa_insn_signal *mem)
 {
   struct BrigInstSignal repr;
 
-  /* This is necessary because of the erroneous typedef of
-     BrigMemoryModifier8_t which introduces padding which may then contain
-     random stuff (which we do not want so that we can test things don't
-     change).  */
   memset (&repr, 0, sizeof (repr));
   repr.base.base.byteCount = lendian16 (sizeof (repr));
   repr.base.base.kind = lendian16 (BRIG_KIND_INST_SIGNAL);
@@ -1344,9 +1340,9 @@ emit_signal_insn (hsa_insn_signal *mem)
   repr.base.type = lendian16 (mem->m_type);
   repr.base.operands = lendian32 (emit_insn_operands (mem));
 
-  repr.memoryOrder = mem->m_memoryorder;
-  repr.signalOperation = mem->m_atomicop;
-  repr.signalType = BRIG_TYPE_SIG64;
+  repr.memoryOrder = mem->m_memory_order;
+  repr.signalOperation = mem->m_signalop;
+  repr.signalType = hsa_machine_large_p () ? BRIG_TYPE_SIG64 : BRIG_TYPE_SIG32;
 
   brig_code.add (&repr, sizeof (repr));
   brig_insn_count++;
@@ -1367,10 +1363,6 @@ emit_atomic_insn (hsa_insn_atomic *mem)
   else
     addr = as_a <hsa_op_address *> (mem->get_op (1));
 
-  /* This is necessary because of the erroneous typedef of
-     BrigMemoryModifier8_t which introduces padding which may then contain
-     random stuff (which we do not want so that we can test things don't
-     change).  */
   memset (&repr, 0, sizeof (repr));
   repr.base.base.byteCount = lendian16 (sizeof (repr));
   repr.base.base.kind = lendian16 (BRIG_KIND_INST_ATOMIC);
@@ -1447,10 +1439,6 @@ emit_alloca_insn (hsa_insn_alloca *alloca)
   struct BrigInstMem repr;
   gcc_checking_assert (alloca->operand_count () == 2);
 
-  /* This is necessary because of the erroneous typedef of
-     BrigMemoryModifier8_t which introduces padding which may then contain
-     random stuff (which we do not want so that we can test things don't
-     change).  */
   memset (&repr, 0, sizeof (repr));
   repr.base.base.byteCount = lendian16 (sizeof (repr));
   repr.base.base.kind = lendian16 (BRIG_KIND_INST_MEM);
@@ -1747,8 +1735,8 @@ emit_queue_insn (hsa_insn_queue *insn)
   repr.base.base.kind = lendian16 (BRIG_KIND_INST_QUEUE);
   repr.base.opcode = lendian16 (insn->m_opcode);
   repr.base.type = lendian16 (insn->m_type);
-  repr.segment = BRIG_SEGMENT_GLOBAL;
-  repr.memoryOrder = BRIG_MEMORY_ORDER_SC_RELEASE;
+  repr.segment = insn->m_segment;
+  repr.memoryOrder = insn->m_memory_order;
   repr.base.operands = lendian32 (emit_insn_operands (insn));
   brig_data.round_size_up (4);
   brig_code.add (&repr, sizeof (repr));
diff --git a/gcc/hsa-dump.c b/gcc/hsa-dump.c
index 3b65684..7e3b9f0 100644
--- a/gcc/hsa-dump.c
+++ b/gcc/hsa-dump.c
@@ -875,9 +875,9 @@ dump_hsa_insn_1 (FILE *f, hsa_insn_basic *insn, int *indent)
       hsa_insn_signal *mem = as_a <hsa_insn_signal *> (insn);
 
       fprintf (f, "%s", hsa_opcode_name (mem->m_opcode));
-      fprintf (f, "_%s", hsa_m_atomicop_name (mem->m_atomicop));
-      if (mem->m_memoryorder != BRIG_MEMORY_ORDER_NONE)
-	fprintf (f, "_%s", hsa_memsem_name (mem->m_memoryorder));
+      fprintf (f, "_%s", hsa_m_atomicop_name (mem->m_signalop));
+      if (mem->m_memory_order != BRIG_MEMORY_ORDER_NONE)
+	fprintf (f, "_%s", hsa_memsem_name (mem->m_memory_order));
       fprintf (f, "_%s ", hsa_type_name (mem->m_type));
 
       dump_hsa_operands (f, mem);
@@ -1106,6 +1106,15 @@ dump_hsa_insn_1 (FILE *f, hsa_insn_basic *insn, int *indent)
 
       dump_hsa_operands (f, insn);
     }
+  else if (hsa_insn_queue *qi = dyn_cast <hsa_insn_queue *> (insn))
+    {
+      fprintf (f, "%s_%s_%s_%s ", hsa_opcode_name (qi->m_opcode),
+	       hsa_seg_name (qi->m_segment),
+	       hsa_memsem_name (qi->m_memory_order),
+	       hsa_type_name (qi->m_type));
+
+      dump_hsa_operands (f, qi);
+    }
   else
     {
       fprintf (f, "%s_%s ", hsa_opcode_name (insn->m_opcode),
diff --git a/gcc/hsa-gen.c b/gcc/hsa-gen.c
index 86620d5..5208dab 100644
--- a/gcc/hsa-gen.c
+++ b/gcc/hsa-gen.c
@@ -1542,10 +1542,9 @@ hsa_insn_mem::operator new (size_t size)
   return obstack_alloc (&hsa_obstack, size);
 }
 
-/* Constructor of class representing atomic instructions and signals.  OPC is
-   the principal opcode, aop is the specific atomic operation opcode.  T is the
-   type of the instruction.  The instruction operands
-   are provided as ARG[0-3].  */
+/* Constructor of class representing atomic instructions.  OPC is the principal
+   opcode, AOP is the specific atomic operation opcode.  T is the type of the
+   instruction.  The instruction operands are provided as ARG[0-3].  */
 
 hsa_insn_atomic::hsa_insn_atomic (int nops, int opc,
 				  enum BrigAtomicOperation aop,
@@ -1572,16 +1571,16 @@ hsa_insn_atomic::operator new (size_t size)
 }
 
 /* Constructor of class representing signal instructions.  OPC is the prinicpal
-   opcode, sop is the specific signal operation opcode.  T is the type of the
+   opcode, SOP is the specific signal operation opcode.  T is the type of the
    instruction.  The instruction operands are provided as ARG[0-3].  */
 
 hsa_insn_signal::hsa_insn_signal (int nops, int opc,
 				  enum BrigAtomicOperation sop,
-				  BrigType16_t t, hsa_op_base *arg0,
-				  hsa_op_base *arg1, hsa_op_base *arg2,
-				  hsa_op_base *arg3)
-  : hsa_insn_atomic (nops, opc, sop, t, BRIG_MEMORY_ORDER_SC_ACQUIRE_RELEASE,
-		     arg0, arg1, arg2, arg3)
+				  BrigType16_t t, BrigMemoryOrder memorder,
+				  hsa_op_base *arg0, hsa_op_base *arg1,
+				  hsa_op_base *arg2, hsa_op_base *arg3)
+  : hsa_insn_basic (nops, opc, t, arg0, arg1, arg2, arg3),
+    m_memory_order (memorder), m_signalop (sop)
 {
 }
 
@@ -1695,8 +1694,13 @@ hsa_insn_comment::~hsa_insn_comment ()
 }
 
 /* Constructor of class representing the queue instruction in HSAIL.  */
-hsa_insn_queue::hsa_insn_queue (int nops, BrigOpcode opcode)
-  : hsa_insn_basic (nops, opcode, BRIG_TYPE_U64)
+
+hsa_insn_queue::hsa_insn_queue (int nops, int opcode, BrigSegment segment,
+				BrigMemoryOrder memory_order,
+				hsa_op_base *arg0, hsa_op_base *arg1,
+				hsa_op_base *arg2, hsa_op_base *arg3)
+  : hsa_insn_basic (nops, opcode, BRIG_TYPE_U64, arg0, arg1, arg2, arg3),
+    m_segment (segment), m_memory_order (memory_order)
 {
 }
 
@@ -4489,11 +4493,10 @@ gen_hsa_insns_for_kernel_call (hsa_bb *hbb, gcall *call)
 
   c = new hsa_op_immed (1, BRIG_TYPE_U64);
 
-  hsa_insn_signal *signal= new hsa_insn_signal (2, BRIG_OPCODE_SIGNALNORET,
-						BRIG_ATOMIC_ST, BRIG_TYPE_B64,
-						signal_reg, c);
-  signal->m_memoryorder = BRIG_MEMORY_ORDER_RELAXED;
-  signal->m_memoryscope = BRIG_MEMORY_SCOPE_SYSTEM;
+  hsa_insn_signal *signal = new hsa_insn_signal (2, BRIG_OPCODE_SIGNALNORET,
+						 BRIG_ATOMIC_ST, BRIG_TYPE_B64,
+						 BRIG_MEMORY_ORDER_RELAXED,
+						 signal_reg, c);
   hbb->append_insn (signal);
 
   /* Get private segment size.  */
@@ -4522,14 +4525,11 @@ gen_hsa_insns_for_kernel_call (hsa_bb *hbb, gcall *call)
   hsa_op_reg *queue_index_reg = new hsa_op_reg (BRIG_TYPE_U64);
 
   c = new hsa_op_immed (1, BRIG_TYPE_U64);
-  hsa_insn_queue *queue = new hsa_insn_queue (3,
-					      BRIG_OPCODE_ADDQUEUEWRITEINDEX);
-
   addr = new hsa_op_address (queue_reg);
-  queue->set_op (0, queue_index_reg);
-  queue->set_op (1, addr);
-  queue->set_op (2, c);
-
+  hsa_insn_queue *queue = new hsa_insn_queue (3, BRIG_OPCODE_ADDQUEUEWRITEINDEX,
+					      BRIG_SEGMENT_FLAT,
+					      BRIG_MEMORY_ORDER_SC_RELEASE,
+					      queue_index_reg, addr, c);
   hbb->append_insn (queue);
 
   /* Get packet base address.  */
@@ -4786,8 +4786,6 @@ gen_hsa_insns_for_kernel_call (hsa_bb *hbb, gcall *call)
     = new hsa_insn_atomic (2, BRIG_OPCODE_ATOMICNORET, BRIG_ATOMIC_ST,
 			   BRIG_TYPE_B32, BRIG_MEMORY_ORDER_SC_RELEASE, addr,
 			   c);
-  atomic->m_memoryscope = BRIG_MEMORY_SCOPE_SYSTEM;
-
   hbb->append_insn (atomic);
 
   /* Ring doorbell signal.  */
@@ -4800,10 +4798,8 @@ gen_hsa_insns_for_kernel_call (hsa_bb *hbb, gcall *call)
   hbb->append_insn (mem);
 
   signal = new hsa_insn_signal (2, BRIG_OPCODE_SIGNALNORET, BRIG_ATOMIC_ST,
-				BRIG_TYPE_B64, doorbell_signal_reg,
-				queue_index_reg);
-  signal->m_memoryorder = BRIG_MEMORY_ORDER_SC_RELEASE;
-  signal->m_memoryscope = BRIG_MEMORY_SCOPE_SYSTEM;
+				BRIG_TYPE_B64, BRIG_MEMORY_ORDER_SC_RELEASE,
+				doorbell_signal_reg, queue_index_reg);
   hbb->append_insn (signal);
 
   /* Prepare CFG for waiting loop.  */
@@ -4829,12 +4825,9 @@ gen_hsa_insns_for_kernel_call (hsa_bb *hbb, gcall *call)
   c = new hsa_op_immed (1, BRIG_TYPE_S64);
 
   signal = new hsa_insn_signal (3, BRIG_OPCODE_SIGNAL,
-				BRIG_ATOMIC_WAIT_LT, BRIG_TYPE_S64);
-  signal->m_memoryorder = BRIG_MEMORY_ORDER_SC_ACQUIRE;
-  signal->m_memoryscope = BRIG_MEMORY_SCOPE_SYSTEM;
-  signal->set_op (0, signal_result_reg);
-  signal->set_op (1, signal_reg);
-  signal->set_op (2, c);
+				BRIG_ATOMIC_WAIT_LT, BRIG_TYPE_S64,
+				BRIG_MEMORY_ORDER_SC_ACQUIRE,
+				signal_result_reg, signal_reg, c);
   new_hbb->append_insn (signal);
 
   hsa_op_reg *ctrl = new hsa_op_reg (BRIG_TYPE_B1);
diff --git a/gcc/hsa.h b/gcc/hsa.h
index d178ebf..f13e216 100644
--- a/gcc/hsa.h
+++ b/gcc/hsa.h
@@ -740,16 +740,21 @@ is_a_helper <hsa_insn_atomic *>::test (hsa_insn_basic *p)
 
 /* HSA instruction for signal operations.  */
 
-class hsa_insn_signal : public hsa_insn_atomic
+class hsa_insn_signal : public hsa_insn_basic
 {
 public:
   hsa_insn_signal (int nops, int opc, enum BrigAtomicOperation sop,
-		   BrigType16_t t, hsa_op_base *arg0 = NULL,
-		   hsa_op_base *arg1 = NULL,
+		   BrigType16_t t, BrigMemoryOrder memorder,
+		   hsa_op_base *arg0 = NULL, hsa_op_base *arg1 = NULL,
 		   hsa_op_base *arg2 = NULL, hsa_op_base *arg3 = NULL);
 
   void *operator new (size_t);
 
+  /* Things like acquire/release/aligned.  */
+  enum BrigMemoryOrder m_memory_order;
+
+  /* The operation itself.  */
+  enum BrigAtomicOperation m_signalop;
 private:
   /* All objects are deallocated by destroying their pool, so make delete
      inaccessible too.  */
@@ -951,10 +956,18 @@ is_a_helper <hsa_insn_comment *>::test (hsa_insn_basic *p)
 class hsa_insn_queue: public hsa_insn_basic
 {
 public:
-  hsa_insn_queue (int nops, BrigOpcode opcode);
+  hsa_insn_queue (int nops, int opcode, BrigSegment segment,
+		  BrigMemoryOrder memory_order,
+		  hsa_op_base *arg0 = NULL, hsa_op_base *arg1 = NULL,
+		  hsa_op_base *arg2 = NULL, hsa_op_base *arg3 = NULL);
 
   /* Destructor.  */
   ~hsa_insn_queue ();
+
+  /* Segment used to refer to the queue.  Must be global or flat.  */
+  BrigSegment m_segment;
+  /* Memory order used to specify synchronization.  */
+  BrigMemoryOrder m_memory_order;
 };
 
 /* Report whether or not P is a queue instruction.  */
@@ -964,7 +977,12 @@ template <>
 inline bool
 is_a_helper <hsa_insn_queue *>::test (hsa_insn_basic *p)
 {
-  return (p->m_opcode == BRIG_OPCODE_ADDQUEUEWRITEINDEX);
+  return (p->m_opcode == BRIG_OPCODE_ADDQUEUEWRITEINDEX
+	  || p->m_opcode == BRIG_OPCODE_CASQUEUEWRITEINDEX
+	  || p->m_opcode == BRIG_OPCODE_LDQUEUEREADINDEX
+	  || p->m_opcode == BRIG_OPCODE_LDQUEUEWRITEINDEX
+	  || p->m_opcode == BRIG_OPCODE_STQUEUEREADINDEX
+	  || p->m_opcode == BRIG_OPCODE_STQUEUEWRITEINDEX);
 }
 
 /* HSA source type instruction.  */
-- 
2.9.0



More information about the Gcc-patches mailing list