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

[Richard Barton]

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