This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: Implement Intel Transactional Synchronization Extensions


>>> On 08.02.12 at 19:25, "H.J. Lu" <hongjiu.lu@intel.com> wrote:
>+static int
>+check_hle (void)
>+{
>+  switch (i.tm.opcode_modifier.hleprefixok)
>+    {
>+    default:
>+      abort ();
>+    case 0:

How about giving these literals proper names. Right now one has to
look up on what instructions they're being use to see what their
intended meaning is.

>+      if (i.prefix[HLE_PREFIX] == XACQUIRE_PREFIX_OPCODE)

With HLE_PREFIX == REP_PREFIX and XACQUIRE_PREFIX_OPCODE
== REPNE_PREFIX_OPCODE, how is this distinguished from the repne
case? Oh, I see, the *caller* checks i.have_hle - that's pretty bad to
be done this way imo.

+	as_bad (_("invalid instruction `%s' after `xacquire'"),
+		i.tm.name);
+      else
+	as_bad (_("invalid instruction `%s' after `xrelease'"),
+		i.tm.name);
+      return 0;
+    case 1:
+      if (i.prefix[LOCK_PREFIX])
+	return 1;
+      if (i.prefix[HLE_PREFIX] == XACQUIRE_PREFIX_OPCODE)
+	as_bad (_("missing `lock' with `xacquire'"));
+      else
+	as_bad (_("missing `lock' with `xrelease'"));

Wouldn't it make sense to insert missing LOCK prefixes silently
(optionally verbosely) rather than complaining about their absence?
(I'd also question the value of displaying both prefixes by default
in the disassembly - this just clutters things rather than helping
with anything.)

+      return 0;
+    case 2:
+      return 1;
+    case 3:
+      if (i.prefix[HLE_PREFIX] != XRELEASE_PREFIX_OPCODE)
+	{
+	  as_bad (_("instruction `%s' after `xacquire' not allowed"),
+		  i.tm.name);
+	  return 0;
+	}
+      if (i.mem_operands == 0
+	  || !operand_type_check (i.types[i.operands - 1], anymem))
+	{
+	  as_bad (_("memory destination needed for instruction `%s'"
+		    " after `xrelease'"), i.tm.name);
+	  return 0;
+	}
+      return 1;
+    }
+}

Further, while the patch deals with CMPXCHG8B, for some reason
it leaves out CMPXCHG16B (perhaps because the instruction, oddly
enough, is considered an SSE3 one).

Finally, albeit consistent with what is documented, I'm surprised that
MOV opcodes 0xA2 and 0xA3 aren't allowed with XRELEASE - is that
really the case?

Jan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]