[llvm-commits] [PATCH] Fix assembly of narrow Thumb data processing instructions

[Richard Barton]

Hi Jim

Thanks for the review.

> This looks great, thank you. You scared me a bit in the description here w/ a
> reference to validateInstruction(), but the actual patch correctly uses
> processInstruction(), so all good. :)

Oh dear, at least I have better patches than descriptions ;}

> Very minor nit: Add periods to the ends of sentences in comments. For one
> example,
> +    // Assemblers should use the narrow encodings of these instructions when
> permissible
> +    // These instructions are special in that they are commutable, so shorter
> encodings
> +    // are available more often

Done.

> Second, it's best not to use R0 in assembler test cases, as that's the default
> value for the encoder, so we won't be able to tell the difference between a
> correct encoding of the operand and the operand not being encoded explicitly
> at all. There are a lot of tests already in the test suite that are horrible
> examples in this regard, unfortunately.

Oh dear, I have moaned about this before in the past as well, so I am a
hypocrite. I have changed most of 
them over to use random non-r0 values. (We still want _some_ r0 values :-)

I have made the suggested adjustments and committed as r159935.

Thanks
Rich



> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com]
> Sent: 06 July 2012 22:47
> To: Richard Barton
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Fix assembly of narrow Thumb data processing instructions
> 
> Hi Richard,
> 
> This looks great, thank you. You scared me a bit in the description here w/ a
> reference to validateInstruction(), but the actual patch correctly uses
> processInstruction(), so all good. :)
> 
> Very minor nit: Add periods to the ends of sentences in comments. For one
> example,
> +    // Assemblers should use the narrow encodings of these instructions when
> permissable
> +    // These instructions are special in that they are commutable, so shorter
> encodings
> +    // are available more often
> 
> Second, it's best not to use R0 in assembler test cases, as that's the default
> value for the encoder, so we won't be able to tell the difference between a
> correct encoding of the operand and the operand not being encoded explicitly
> at all. There are a lot of tests already in the test suite that are horrible
> examples in this regard, unfortunately.
> 
> Good to commit w/ those fixes.
> 
> -Jim
> 
> On Jul 4, 2012, at 11:43 AM, Richard Barton <richard.barton at arm.com> wrote:
> 
> > Hello Reviewers
> >
> > The attached patch implements correct assembly of Thumb dp-operations with
> both
> > three and two register syntaxes.
> >
> > i.e. AND, EOR, LSL, LSR, ASR, ADC, SBC, ROR, and ORR.
> >
> > The assembly syntax for these instructions is of the form:
> >
> > AND{S}{<c>}{<q>}  {<Rd>,} <Rn>, <Rm> {, <shift>}
> >
> > The optional Rd argument allows the use of the short encoding when Rd == Rn
> (and
> > other conditions hold.) The assembler should always prefer the short
> encoding
> > over the wide in these cases, unless the wide encoding is specifically
> requested
> > with '.W'.
> >
> > In addition, ADD, EOR, ADC and ORR are commutative, so the narrow encoding
> is
> > also available when Rd == Rm (as well as the other conditions holding.)
> >
> > The additional conditions are:
> > - Rd, Rn and Rm are all in the range R0 - R7.
> > - The instruction is flag-setting and not in an IT block, or not-flag
> setting
> > and in an IT block.
> > - No shift is applied to the instruction.
> >
> > The attached patch uses the validateInstruction callback to chose the narrow
> > encodings in the correct circumstances, and adds a new regression test.
> >
> > Please review.
> >
> > Regards,
> > Richard Barton
> > ARM Ltd, Cambridge
> >
> > <thumb2-narrow-dp-assembly.patch>
> 
>