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

[Jim Grosbach]

On Sep 20, 2012, at 5:03 PM, Greg Fitzgerald <garious at gmail.com> wrote:

>> + "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.
> 

This is correct.


> 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?

That would be a bug in ISel.

The InstPrinter and the MCCodeEmitter should probably have assert()s in there to validate the range of the immediate operand to help catch such bugs, but I suspect they don't.

-Jim

> 
> -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.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits