[llvm-commits] [PATCH] Fix decoding of shift-immediate #32

[Jim Grosbach]

Hi Richard,

This looks fine, with the exception that I'd prefer to keep an assert() in the emitter, but just change it to check that the value is < 32. Ok to commit w/ that change.

-Jim

On Mar 14, 2012, at 9:03 AM, Richard Barton <richard.barton at arm.com> wrote:

> Hi Jim
> 
> Thanks for the review and the clarification on the MCInst contents, I will bear this rule-of-thumb in mind in future.
> 
> I have reversed my patch to correct the AsmParser behaviour to store #0 in the MCInst rather than #32, and have removed the special case handling in the CodeEmitter for #32.
> 
> Please review.
> 
> Thanks
> Rich
> 
>> -----Original Message-----
>> From: Jim Grosbach [mailto:grosbach at apple.com]
>> Sent: 13 March 2012 23:18
>> To: Richard Barton
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Fix decoding of shift-immediate #32
>> 
>> Hi Richard,
>> 
>> MC tries to represent instruction operands with the values that will be
>> encoded into the instruction, not the semantic meaning. The arm backend isnt
>> yet completely consistent about it, but that's the general idea. In this case,
>> that means we should use an mcoperand value of zero, as that's what the
>> encoded value is.
>> 
>> Anything creating an operand value of 32 for this is in error. It's getting
>> away with it because the encoder will just use the low 5 bits, but it's still
>> undesirable to rely on that.
>> 
>> Jim
>> 
>> On Mar 13, 2012, at 11:16 AM, Richard Barton <richard.barton at arm.com> wrote:
>> 
>>> With the patch attached this time! Apologies.
>>> 
>>>> Hello reviewers,
>>>> 
>>>> An MCInst stores a register-shifted-immediate as 2 operands, a register and
>> an
>>>> immediate operand describing the shift kind and the shift value. This
>>>> immediate operand value is encoded as 3 bits for the shift kind, which is
>> an
>>>> enum type ShiftOpc, and 5 bits for the shift value itself. The assembler
>>>> creates an MCInst immediate operand with the value #32 for ... lsr #32,
>> whilst
>>>> the decoder creates an MCInst immediate operand with the value 0 for the
>>>> equivalent encoding. On the face of it, neither is more right than the
>> other,
>>>> and crucially the two paths followed by llvm-mc (assemble, encode) and
>>>> (decode, disassemble) match up in their expectations.
>>>> 
>>>> Although it probably does not matter either way, intuitively I think it
>> makes
>>>> more sense for the decoder to write an MCInst operand describing the shift
>> as
>>>> [32][lsr] rather than [0][lsr] as the latter is not actually legal.
>>>> 
>>>> The attached patch handles this case for the decoder. It also reverts part
>> of
>>>> r137322, which added special case handling for the old decoder behaviour in
>>>> the instruction printer.
>>>> 
>>>> Please review.
>>>> 
>>>> Thanks,
>>>> 
>>>> Richard Barton
>>> <decode_lsr_asr_32.patch>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> 
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.<incorrect_assert_in_shift_encode.patch>