[PATCH] ARM: Add support for ARM modified immediate syntax - Take 3

[Asiri Rathnayake]

Hi Tim,

> ================
> Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:4463-4465
> @@ +4462,5 @@
> +
> +  if (Parser.getTok().isNot(AsmToken::Comma)) {
> +    // Consider [mov r0, #-10], which is aliased with mvn. We cannot fail
> +    // the parse here.
> +    Operands.push_back(ARMOperand::CreateImm(Imm1Exp, Sx1, Ex1));
> ----------------
> I don't understand this one. Wouldn't that example have been covered by AsmToken::EndOfStatement above?

-10 (0xfffffff6 in two's complement) is not representable as a modified immediate. Since we have already parsed the input and cannot back-out at this stage, here we create a plain immediate and let other rules match it. I'm not 100% sure of the tablegen blackmagic going underneath, but it's the following instruction alias that consumes the immediate generated this way:

def : ARMInstAlias<"mov${s}${p} $Rd, $imm",
                   (MVNi rGPR:$Rd, mod_imm_not:$imm, pred:$p, cc_out:$s)>;

I think this alias definition causes the immediate to be treated as -(-10), which can be encoded as a modified immediate (+10).

> 
> ================
> Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:4502-4506
> @@ +4501,7 @@
> +    Imm2 = CE->getValue();
> +    if (!(Imm2 & ~0x1E)) {
> +      // We have a match!
> +      unsigned Enc = ((Imm2 & 0x1E) << 7) | Imm1;
> +      Operands.push_back(ARMOperand::CreateModImm((Enc & 0xFF),
> +                                                  (Enc & 0xF00) >> 7,
> +                                                  S, Ex2));
> ----------------
> This looks a bit sketchy: combining and then splitting apart the immediates with some redundant masks.
> 
> Doesn't it amount to "CreateModImm(Imm1, Imm2)"?

Fixed :)

> 
> ================
> Comment at: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp:1336
> @@ +1335,3 @@
> +  int32_t Rotated = ARM_AM::rotr32(Bits, Rot);  if ((Bits & 0x3) && 
> + (Rotated & ~0xFF)) {
> +    // #rot has the least possible value
> ----------------
> I don't think this is quite right. E.g. Op.getImm() == 0.
> 
> The obviously correct test is that "getSOImmVal(Rotated) == Op.getImm()", isn't it? If you think that's too expensive, could we at least assert it?

Op.getImm() == 0 will be taken care of from the previous if statement (Rot == 0), but I think your version is much more readable, I've switched to it.

OK to commit?

Thanks!

- Asiri

> 
> http://reviews.llvm.org/D6408