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

[Tim Northover]

Hi Asiri,

Sorry for the delay here. I've got some minor quibbles, but it looks mostly OK:

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

================
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)"?

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

http://reviews.llvm.org/D6408