[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