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

[Jim Grosbach]

Hi Richard,

The alias of a shift by zero to a mov should be handled entirely in either the assembler or instruction selection (for the assembler and the compiler, respectively). Anything that's actually creating a shift instruction w/ a shift value of zero is doing it wrong, and that's a bug that should be fixed.

-Jim

On Mar 20, 2012, at 5:41 AM, Richard Barton <richard.barton at arm.com> wrote:

> Hi Jim
> 
> Wow, this is the patch that just does not want to work.
> 
> I reinstated the assert and found that this causes a test suite failure in
> basic-arm-instructions.s.
> 
> It seems that it is legal in UAL to use "asr r2,r4,#0" to mean "mov r2,r4". So
> using 0 in the MCInst seems to confuse the encoder into thinking that "asr
> r2,r4,#32" is a mov.
> 
> I think that the easiest way forward is to revert to the original patch and
> store 32 in the MC Inst when we mean #32 (i.e. for asr and lsr) and 0 when we
> mean #0.
> 
> This passes the llvm test suite and passes our internal tests for AND with
> shifted registers and ASR.
> 
> I attached the new proposed patch.
> 
> Thanks
> Rich
> 
> 
>> -----Original Message-----
>> From: Jim Grosbach [mailto:grosbach at apple.com]
>> Sent: 16 March 2012 18:14
>> To: Richard Barton
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Fix decoding of shift-immediate #32
>> 
>> 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>
>> 
>> 
> <register_shifted_by_32.patch>