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

Jim Grosbach grosbach at apple.com
Wed Apr 25 09:34:46 PDT 2012


Hi Richard,

Looking much better now. Thanks!

> Index: lib/Target/ARM/AsmParser/ARMAsmParser.cpp
> ===================================================================
> --- lib/Target/ARM/AsmParser/ARMAsmParser.cpp	(revision 153578)
> +++ lib/Target/ARM/AsmParser/ARMAsmParser.cpp	(working copy)
> @@ -1412,8 +1412,10 @@
>      assert(isRegShiftedImm() &&
>             "addRegShiftedImmOperands() on non RegShiftedImm!");
>      Inst.addOperand(MCOperand::CreateReg(RegShiftedImm.SrcReg));
> +    // Shift of #32 is encoded as 0 where permitted
> +    unsigned Imm = (RegShiftedImm.ShiftImm == 32 ? 0 : RegShiftedImm.ShiftImm);
>      Inst.addOperand(MCOperand::CreateImm(
> -      ARM_AM::getSORegOpc(RegShiftedImm.ShiftTy, RegShiftedImm.ShiftImm)));
> +      ARM_AM::getSORegOpc(RegShiftedImm.ShiftTy, Imm)));
>    }
>  
>    void addShifterImmOperands(MCInst &Inst, unsigned N) const {
> @@ -6718,6 +6720,9 @@
>      // A shift by zero is a plain MOVr, not a MOVsi.
>      unsigned Amt = Inst.getOperand(2).getImm();
>      unsigned Opc = Amt == 0 ? ARM::MOVr : ARM::MOVsi;
> +    // A shift by 32 should be encoded as 0 when permitted
> +    if (Amt == 32 && (ShiftTy == ARM_AM::lsr || ShiftTy == ARM_AM::asr))
> +        Amt = 0;

Indentation should be just two spaces after the 'if'.

>      unsigned Shifter = ARM_AM::getSORegOpc(ShiftTy, Amt);
>      MCInst TmpInst;
>      TmpInst.setOpcode(Opc);
> @@ -7037,7 +7042,9 @@
>    }
>    case ARM::MOVsi: {
>      ARM_AM::ShiftOpc SOpc = ARM_AM::getSORegShOp(Inst.getOperand(2).getImm());
> -    if (SOpc == ARM_AM::rrx) return false;
> +    // rrx shifts and asr/lsr of #32 is encoded as 0
> +    if (SOpc == ARM_AM::rrx || SOpc == ARM_AM::asr || SOpc == ARM_AM::lsr) 
> +      return false;

Do we still accept syntactically a shift of zero and transform it correctly?

>      if (ARM_AM::getSORegOffset(Inst.getOperand(2).getImm()) == 0) {
>        // Shifting by zero is accepted as a vanilla 'MOVr'
>        MCInst TmpInst;
> Index: lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
> ===================================================================
> --- lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp	(revision 153578)
> +++ lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp	(working copy)
> @@ -1188,8 +1188,7 @@
>    // Encode shift_imm bit[11:7].
>    Binary |= SBits << 4;
>    unsigned Offset = ARM_AM::getSORegOffset(MO1.getImm());
> -  assert(Offset && "Offset must be in range 1-32!");
> -  if (Offset == 32) Offset = 0;
> +  assert(Offset < 32 && "Offset must be in range 0-31!");
>    return Binary | (Offset << 7);
>  }
>  





On Mar 28, 2012, at 10:00 AM, Richard Barton <richard.barton at arm.com> wrote:

> Hi Jim
> 
> Apologies for the delays in my responses to your review comments, I'm juggling a
> few things at the moment.
> 
> Attached is the new patch with the previously agreed changes to parsing shifter
> operands and the assertion in the encoder. I have also added to the patch to
> change the way the shift pseudo-instructions are handled to also store 0 for
> asr/lsr #32.
> 
> New patch attached.
> 
> Thanks
> Rich
> 
> 
>> -----Original Message-----
>> From: Jim Grosbach [mailto:grosbach at apple.com]
>> Sent: 21 March 2012 22:54
>> To: Richard Barton
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Fix decoding of shift-immediate #32
>> 
>> 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>
>> 
>> 
> <shift_by_32.patch>




More information about the llvm-commits mailing list