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

[Tim Northover]

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.