Re: PATCH: Implement Intel Transactional Synchronization Extensions

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Feb 21, 2012 at 12:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> 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.

I checked in a patch to defineHLEPrefixNone, HLEPrefixLock,
HLEPrefixAny and HLEPrefixRelease.

>>+ ? ? ?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.

Any suggestions are welcome.

> + ? ? ? 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.)

In some cases, it is very useful to know/control exactly how many
prefixes are generated.

> + ? ? ?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?
>

Yes, they are implemented according to TSX spec.


-- 
H.J.