Home Original page

PATCH: Implement Intel Transactional Synchronization Extensions

Jan Beulich JBeulich@suse.com
Tue Feb 21 08:57:00 GMT 2012 
>>> 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
More information about the Binutils mailing list