[llvm-commits] [PATCH] Fix decoding of shift-immediate #32
Richard Barton
richard.barton at arm.com
Wed Apr 25 11:20:36 PDT 2012
I quickly committed it as r155565 before even thinking about your other question
;)
> I'm almost scared to ask this follow up question, but do we get this right for
> the Thumb2 instructions or do we need a similar fix there, too?
I ran our test suite over the 16 and 32 bit Thumb ASR instructions and they all
passed. Running llvm-mc over one example seems to suggest that they are behaving
the same as the ARM ones. (see below.)
Thanks for the review, feels good to get this one in.
Ta,
Rich
19:16:30:/work/ricbar01/llvm> echo '0x5f 0xea 0x20 0x0e' | ./build/bin/llvm-mc
-triple thumbv7 -show-inst -show-encoding -disassemble
.section __TEXT,__text,regular,pure_instructions
asrs.w lr, r0, #32 @ encoding: [0x5f,0xea,0x20,0x0e]
@ <MCInst #2216 t2ASRri
@ <MCOperand Reg:40>
@ <MCOperand Reg:60>
@ <MCOperand Imm:0>
@ <MCOperand Imm:14>
@ <MCOperand Reg:0>
@ <MCOperand Reg:2>>
19:17:09:/work/ricbar01/llvm> echo 'asrs.w lr, r0, #32' | ./build/bin/llvm-mc
-triple thumbv7 -show-inst -show-encoding -assemble
.section __TEXT,__text,regular,pure_instructions
asrs.w lr, r0, #32 @ encoding: [0x5f,0xea,0x20,0x0e]
@ <MCInst #2216 t2ASRri
@ <MCOperand Reg:40>
@ <MCOperand Reg:60>
@ <MCOperand Imm:0>
@ <MCOperand Imm:14>
@ <MCOperand Reg:0>
@ <MCOperand Reg:2>>
> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com]
> Sent: 25 April 2012 18:51
> To: Richard Barton
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Fix decoding of shift-immediate #32
>
> Cool. Looks good to me! Please commit.
>
> I'm almost scared to ask this follow up question, but do we get this right for
> the Thumb2 instructions or do we need a similar fix there, too?
>
> -Jim
>
> On Apr 25, 2012, at 10:45 AM, Richard Barton <Richard.Barton at arm.com> wrote:
>
> > Hi Jim
> >
> > Thanks for the review.
> >
> >> Indentation should be just two spaces after the 'if'.
> > Done
> >
> >> Do we still accept syntactically a shift of zero and transform it
> correctly?
> >
> > Any shifts by #0 are converted into MOVr instructions before we convert do
> our encoding trick with the shifts by #32. Thus any MOVsi with a right shift
> by #0 must be a right shift by #32 and should not be converted into a MOVr.
> >
> > Updated patch attached (with the indentation change.)
> >
> > Thanks
> > Rich
> >
> >
> >> -----Original Message-----
> >> From: Jim Grosbach [mailto:grosbach at apple.com]
> >> Sent: 25 April 2012 17:35
> >> To: Richard Barton
> >> Cc: llvm-commits at cs.uiuc.edu
> >> Subject: Re: [llvm-commits] [PATCH] Fix decoding of shift-immediate #32
> >>
> >> 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>
> >>
> >>
> >
> >
> > -- 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.<shift_by_32.patch>
>
>
More information about the llvm-commits
mailing list