[llvm-commits] [PATCH] load/store shift fix

[Greg Fitzgerald]

> + "lsr #32" gives an MCInst immediate of 0. Encoder and Decoder can be
> as naive as you like. Printer, parser and selection have to be
> special.
>
> I think the second is more in line with the goals of the MC layer.

That'd be fine, but what are you doing to prevent the MCOperand from
being constructed with #32?  Your patch guards llvm-mc from doing it
in the AsmParser, but what about llc?

-Greg


On Thu, Sep 20, 2012 at 1:40 PM, Tim Northover <t.p.northover at gmail.com> wrote:
> Hi Greg,
>
> I'm afraid I disagree with this issue too.
>
>> One small concern however, is that your patch catches shifts of #32 in
>> the parser and not during encoding.
>>
>> +    if (Imm == 32)
>> +      Imm = 0;
>>
>> This hides a subtle bug ARMMCCodeEmitter that can't be detected from
>> llvm-mc once your patch is in place.  LLVM's instruction selection
>> doesn't use the ASCII gateway to creating MCInst objects, and instead
>> creates them directly.  If it were to create one with #32, which
>> requires 6 bits instead of 5 available to encode it, then the code
>> emitter will foolishly go out of bounds and write over the adjacent
>> field.
>
> This all comes down to what we want the canonical MCInst for these
> instructions to be. Our two options are:
>
> + "lsr #32" (etc) gives an MCInst immediate of 32. Encoder and decoder
> are special (if the decoder isn't special, MCInsts are no longer
> canonical since "lsr #32" will be decoded to an immediate of 0).
> Printer and Parser can forget about it. Selection equally special
> here: "lsr #0" needs special handling to be converted to an MCOperand
> of 0.
> + "lsr #32" gives an MCInst immediate of 0. Encoder and Decoder can be
> as naive as you like. Printer, parser and selection have to be
> special.
>
> I think the second is more in line with the goals of the MC layer.
>
> Tim.