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

Richard Barton Richard.Barton at arm.com
Wed Apr 25 10:45:23 PDT 2012


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shift_by_32.patch
Type: application/octet-stream
Size: 2315 bytes
Desc: shift_by_32.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120425/a530c62b/attachment.obj>


More information about the llvm-commits mailing list