[PATCH v2] X86: disambiguate unqualified btr, bts

[Jim Grosbach]

On Jul 15, 2013, at 7:25 PM, Stephen Checkoway <s at pahtak.org> wrote:

> 
> On Jul 15, 2013, at 9:49 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 
>> Yeah, that’s the best reference I found, but there may be better. If there is, I’d love to see it (and have a link to it on the llvm.org pages somewhere).
> 
> I actually only got interested in this thread because you posted that link. I was interested in seeing a reference since I'd never come across one before. 
> 
>>> 2a. btq $n, mem for n in [32, 63] or
>>> 2b. btl $(n-32), mem+4 for n in [32, 63]
>> 
>> Personally, 2a is least surprising. 2b will result in an instruction in a disassembly that has both a different immediate and a different memory reference than the instruction I wrote, which would very much surprise me.
> 
> I too think it would be surprising if I were to disassemble the resulting binary and find something totally different looking. Whether or not I'd find that more surprising than pseudo instructions for rlwinm (rotate left word immediate then AND with mask) on ppc such as extlwi or rotrwi disassembling as rlwinm, I'm not sure.
> 
>> That said, I would tend to prefer this case simply generate a diagnostic saying the immediate is out of range for btl, perhaps with a fixit style notice to use btq or a btl w/ mem+4, but I can see the usability argument the other way. IIUC, a diagnostic is what that Solaris doc would get here since it uses an ‘l’ suffix by default.
> 
> That seems like a perfectly reasonable thing to do.

Cool. Sounds like we have a winner, then. Default to ‘l’ and issue a diagnostic for immediates outside the range [0,31].

It’s worth noting that this is distinct from, but definitely related to, the previous PR was asking for something more aggressive using extra information from the context surrounding the inline assembly, at least if I’m reading things correctly. This proposed change is restricted to the assembler and will give consistent behavior for assembly written as inline asm and as standalone .s files, which is general goodness.

-Jim

> 
>> Do you have a preference? From Linus’ description, the Linux kernel will always be in the (1) case due to their choice of constraint, so this is outside the scope of kernel expectations.
> 
> My only preference would be to not silently accept values out of the range. It should either work like the register, memory form and test the nth bit starting from the bit base or it should generate a diagnostic saying the immediate is out of range.
> 
>>> Or even take the more general route discussed in the Intel manual
>>> bt $n, mem becomes btl $(n % 32), mem + 4*(n / 32)
>> 
>> This scares the crap out of me. :)
> 
> =)
> 
>>> bt, btr, and bts should probably operate in the same manner.
>> 
>> Agreed. Probably btc also? Any others?
> 
> Oh yes, btc too. I can't think of others off hand.
> 
>> 
>>> It seems unlikely people would ever write an immediate value outside the allowed range and want/expect the compiler to blindly copy the one byte immediate even though the hardware only looks at the least significant 4 (resp. 5 or 6) bits for btw (resp. btl or btq).
>> 
>> Also agreed. An out-of-range immediate is almost certainly either a) user error or b) user expecting the assembler to be clever. For a well-selected definition of a “reasonable range,” it’s probably safe to assume (b) and indeed be clever. Either [0,31] or [0,63] seem to fit the bill for that range.
> 
> 
> One advantage of assuming user error is the decision can be revised down the road if it turns out there really are people who rely on the assembler being clever. If the assembler is made clever now, it may be a decision llvm needs to stick with down the road if people start to rely on it.
> 
> -- 
> Stephen Checkoway

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130716/2b27a338/attachment.html>