llvm dev - Jul 2013 - [LLVMdev] [PATCH] x86: disambiguate unqualified btr, bts

Eli Friedman wrote:
> The reason it's the right thing to do is that the mem/imm forms of
> btsw and btsl have exactly the same semantics.
The Intel documentation implies that this is the case:
> If the bit base operand specifies a memory location, it represents the
address of the byte in memory that contains the bit base (bit 0 of the specified
byte) of the bit string (see Figure 3-2). The offset operand then selects a bit
position within the range −231 to 231 − 1 for a register offset and 0 to 31 for
an immediate offset.
However, this doesn't seem to be true if the immediate value is greater than
15. See the attached imm.c which prints the results of bts m16,imm8 (btsw) and
bts m32,imm8 (btsl) with the immediate in [0,63]. For btsw, only the least
significant 4 bits of the immediate seem to be used whereas for btsl, only the
least significant 5 bits seem to be used.

In contrast, bts m16,r16 (btsw) and bts m32,r32 (btsl) are identical for bit
offset operands in [0,63] and are likely identical for [-2^{15}, 2^{15}-1],
although I didn't actually check the others. See the attached reg.c which
changes the immediate constraints to register constraints.

btr behaves similarly.

For the memory, immediate form without the suffix, it seems like the options are
1. If the immediate value is in [0,15], use btsl/btrl since it saves a byte,
otherwise error;
2. Follow both gas's behavior and the Solaris assembler manual Jim Grosbach
linked to which stated that unsuffixed instructions are assumed to be long and
alias bts to btsl and btr to btrl; or
3. Always error even when there is no ambiguity.

I have no opinion on which option LLVM should follow.

-- 
Stephen Checkoway



-------------- next part --------------
A non-text attachment was scrubbed...
Name: imm.c
Type: application/octet-stream
Size: 576 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130713/1669e7a7/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reg.c
Type: application/octet-stream
Size: 578 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130713/1669e7a7/attachment-0001.obj>

Stephen Checkoway wrote:
> [...]
Thanks for the absolutely splendid analysis!
> For the memory, immediate form without the suffix, it seems like the
options are
> 1. If the immediate value is in [0,15], use btsl/btrl since it saves a
byte, otherwise error;
> 2. Follow both gas's behavior and the Solaris assembler manual Jim
Grosbach linked to which stated that unsuffixed instructions are assumed to be
long and alias bts to btsl and btr to btrl; or
> 3. Always error even when there is no ambiguity.
>
> I have no opinion on which option LLVM should follow.
Okay, so while digging through the history of the linux.git tree, I
found this.  The patch _replaces_ btrl instructions with btr
instructions, for seemingly good reason.  What is your opinion on the
issue?

commit 1c54d77078056cde0f195b1a982cb681850efc08
Author: Jeremy Fitzhardinge <jeremy at goop.org>
Date:   5 years ago

    x86: partial unification of asm-x86/bitops.h

    This unifies the set/clear/test bit functions of asm/bitops.h.

    I have not attempted to merge the bit-finding functions, since they
    rely on the machine word size and can't be easily restructured to work
    generically without a lot of #ifdefs.  In particular, the 64-bit code
    can assume the presence of conditional move instructions, whereas
    32-bit needs to be more careful.

    The inline assembly for the bit operations has been changed to remove
    explicit sizing hints on the instructions, so the assembler will pick
    the appropriate instruction forms depending on the architecture and
    the context.

    Signed-off-by: Jeremy Fitzhardinge <jeremy at xensource.com>
    Cc: Andi Kleen <ak at suse.de>
    Cc: Linus Torvalds <torvalds at linux-foundation.org>

    Signed-off-by: Ingo Molnar <mingo at elte.hu>
    Signed-off-by: Thomas Gleixner <tglx at linutronix.de>

> The patch _replaces_ btrl instructions with btr
> instructions, for seemingly good reason.  What is your opinion on the
> issue?
Mine is it would be a sympathetic reason if correct, but not good if
the instructions shouldn't exist in the first place. However:

(From the commit message):
>     The inline assembly for the bit operations has been changed to remove
>     explicit sizing hints on the instructions, so the assembler will pick
>     the appropriate instruction forms depending on the architecture and
>     the context.
It doesn't do this, as far as I can tell. I cannot make the unsuffixed
versions do anything except an 'l' access in either gas or gcc,
regardless of architecture, pointer size or anything else.

What the code actually does is produce "btsl $imm, addr" if 0 <=
imm
<=31 (at compile-time) and "btsl %eax, addr" otherwise, which works
even for 64-bit types (by Stephen's analysis), but involves an extra
"movl $imm, %eax".
>From the commit message, it seems like they *do* intend it to produce
"btslq $imm, addr" where possible and might well be open to fixing
this as a bug (that just happens to help Linux compile with Clang)
rather than purely for toolchain compatibility.

That said, it's not clear how to enable both forms to be generated
easily from inline asm, but I'm not an expert.

Tim.