[AArch64] Allow negative immediates for ADD/SUB in asm

[rengolin]

Summary

Allow assembly code (either in an asm file or inline asm) to have negative
offsets for ADD/SUB, and convert them to their inverse instructions as per
the ARMv8 Instruction Set Overview, section 5.4.1.

I'm not really sure of a few things:

1. If I accept negative numbers in isAddSubImm(), will this break other stuff?
2. The manual doesn't say specifically, but I'm assuming we can't swap ADDS/SUBS.
3. The error message may not be in the best of places, nor the best of texts. I'm open to suggestions.
4. Is this really the best way to modify the MCInst?
5. Adding support for #-4096 and higher powers of two may be trivial. I'll check while this review goes on.
6. Changing the parser like this seems a lot better than both printers (as I had in bugzilla).

Fixes PR20978.

[t.p.northover]

Can't this be done as a simple InstAlias? The operand type even seems to exist already to support CodeGen (neg_addsub_shifted_imm, though it's a bit weird). You'd need a slightly adapted isNegAddSubImm and addNegAddSubImm methods, but you'd get your shifted values practically for free.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

3324–3325

The immediate form, we probably can. As I recall the edge cases where the transformation is invalid occur when the RHS is U?INT_(MIN|MAX). The shifted 12-bit immediates never are, so it should be OK.

[rengolin]

In D10810#196491, @t.p.northover wrote:

Can't this be done as a simple InstAlias? The operand type even seems to exist already to support CodeGen (neg_addsub_shifted_imm, though it's a bit weird). You'd need a slightly adapted isNegAddSubImm and addNegAddSubImm methods, but you'd get your shifted values practically for free.

This was my first attempt, but since this is the asm parser, the instruction can be matched, but will not change. Both in asm files and inline asm, the parser is simply building MCInsts from whatever comes.

Unless I'm missing something obvious... Do you have an example in the table-gen file that does that already?

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

3324–3325

Good point.

[aadg]

You might want to have a look at the patch which I submitted to the mailing list On July 11th, 2014 ("[PATCH] [AArch64] Add add/sub/cmp/cmn aliases to MC AsmParser") to solve this very issue. It uses a different approach --- mostly tablegen based.

[rengolin]

In D10810#196624, @aadg wrote:

You might want to have a look at the patch which I submitted to the mailing list On July 11th, 2014 ("[PATCH] [AArch64] Add add/sub/cmp/cmn aliases to MC AsmParser") to solve this very issue. It uses a different approach --- mostly tablegen based.

Hum, I remember that patch. I thought you had committed... If that one fixes the issue, do you want to go ahead and commit it?

[rengolin]

Abandoning, since Arnaud had a better solution all along.

[aadg]

In D10810#197716, @rengolin wrote:

Abandoning, since Arnaud had a better solution all along.

Committed my patch @ r241166.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

           || ELFRefKind == AArch64MCExpr::VK_TPREL_HI12
           || ELFRefKind == AArch64MCExpr::VK_TPREL_LO12
           || ELFRefKind == AArch64MCExpr::VK_TPREL_LO12_NC
           || ELFRefKind == AArch64MCExpr::VK_TLSDESC_LO12;
     }
 
     // Otherwise it should be a real immediate in range:
     const MCConstantExpr *CE = cast<MCConstantExpr>(Expr);
-    return CE->getValue() >= 0 && CE->getValue() <= 0xfff;
+    // Negative values are accepted as long as they can be transformed
+    // into a valid SUB/ADD operands.
+    return CE->getValue() >= -(0xfff) && CE->getValue() <= 0xfff;
   }
   bool isCondCode() const { return Kind == k_CondCode; }
   bool isSIMDImmType10() const {
     if (!isImm())
       return false;
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
     if (!MCE)
       return false;

     Parser.eatToEndOfStatement();
     return Error(Loc, "unexpected token in argument list");
   }
 
   Parser.Lex(); // Consume the EndOfStatement
   return false;
 }
 
+// Get reverse Opcode for ADD/SUB due to negative immediates
+// if it is semantically possible to swap them.
+static unsigned getAddSubReverseOpp(unsigned Opcode) {
+  switch(Opcode) {
+  default:
+    return 0;
+  // FIXME: Can we swap those?
+  case AArch64::ADDSWri:
+  case AArch64::ADDSXri:
+  case AArch64::SUBSWri:
+  case AArch64::SUBSXri:
+    return 0;
+  case AArch64::ADDWri:
+    return AArch64::SUBWri;
+  case AArch64::ADDXri:
+    return AArch64::SUBXri;
+  case AArch64::SUBWri:
+    return AArch64::ADDWri;
+  case AArch64::SUBXri:
+    return AArch64::ADDXri;
+  }
+}
+
 // FIXME: This entire function is a giant hack to provide us with decent
 // operand range validation/diagnostics until TableGen/MC can be extended
 // to support autogeneration of this kind of validation.
 bool AArch64AsmParser::validateInstruction(MCInst &Inst,
                                          SmallVectorImpl<SMLoc> &Loc) {
   const MCRegisterInfo *RI = getContext().getRegisterInfo();
   // Check for indexed addressing modes w/ the base register being the
   // same as a destination/source register or pair load where

   case AArch64::ADDWri:
   case AArch64::ADDXri:
   case AArch64::SUBSWri:
   case AArch64::SUBSXri:
   case AArch64::SUBWri:
   case AArch64::SUBXri: {
     // Annoyingly we can't do this in the isAddSubImm predicate, so there is
     // some slight duplication here.
+    if (Inst.getOperand(2).isImm()) {
+      // ADD/SUB can safely swap if their operands are negative
+      // ARMv8 Instruction Set Overview 5.4.1
+      int Imm = Inst.getOperand(2).getImm();
+      if (Imm < 0 && Imm > -4096) {
+        unsigned ReverseOp = getAddSubReverseOpp(Inst.getOpcode());
+        if (!ReverseOp)
+          return Error(Loc[2],
+                       "negative immediate only in ADD/SUB up to -4095");
+        // FIXME: Better way of re-writing the instruction?
+        auto It = Inst.begin() + 2;
+        Inst.setOpcode(ReverseOp);
+        Inst.erase(It);
+        Inst.insert(It, MCOperand::createImm(-Imm));
+        return false;
+      }
+    }
     if (Inst.getOperand(2).isExpr()) {
       const MCExpr *Expr = Inst.getOperand(2).getExpr();
       AArch64MCExpr::VariantKind ELFRefKind;
       MCSymbolRefExpr::VariantKind DarwinRefKind;
       int64_t Addend;
       if (!classifySymbolRef(Expr, ELFRefKind, DarwinRefKind, Addend)) {
         return Error(Loc[2], "invalid immediate expression");
       }

test/MC/AArch64/addsub-inline-asm.ll

+; RUN: llc %s -o - | FileCheck %s
+; ModuleID = 'a64.c'
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64"
+
+define i32 @main(i32 %argc, i8** nocapture readnone %argv) {
+entry:
+; CHECK-LABEL: main
+  %0 = tail call i32 asm sideeffect "add ${0:w}, ${0:w}, ${1:w}\09\0A", "=&r,IJr,~{memory}"(i32 -1024) #1
+; CHECK: sub{{.*}}#1024
+  %1 = tail call i32 asm sideeffect "sub ${0:w}, ${0:w}, ${1:w}\09\0A", "=&r,IJr,~{memory}"(i32 -4095) #1
+; CHECK: add{{.*}}#4095
+  ret i32 %1
+}
+

test/MC/AArch64/addsub.s

+// RUN: llvm-mc -arch aarch64 %s | FileCheck %s
+
+  .text
+  .globl  main
+  .align  2
+  .type  main,@function
+main:
+  //CHECK: add w0, w0, #0
+  add w0, w0, #0
+  //CHECK: sub w0, w0, #1
+  add w0, w0, #-1
+  //CHECK: sub w0, w0, #1024
+  add w0, w0, #-1024
+  //CHECK: sub w0, w0, #2048
+  add w0, w0, #-2048
+  //CHECK: sub w0, w0, #4095
+  add w0, w0, #-4095
+  //CHECK: sub w4, w5, #1, lsl #12
+  add w4, w5, #-1, lsl #12
+  // FIXME: Add support for
+  // add w0, w0, #-4096
+  // To translate to:
+  // sub w0, w0, #1, lsl #12
+  // and vice-versa
+
+  //CHECK: add w0, w0, #1, lsl #12
+  add w0, w0, #4096
+
+  //CHECK: sub w0, w0, #0
+  sub w0, w0, #0
+  //CHECK: add w0, w0, #1
+  sub w0, w0, #-1
+  //CHECK: add w0, w0, #1024
+  sub w0, w0, #-1024
+  //CHECK: add w0, w0, #2048
+  sub w0, w0, #-2048
+  //CHECK: add w0, w0, #4095
+  sub w0, w0, #-4095
+  //CHECK: add w4, w5, #1, lsl #12
+  sub w4, w5, #-1, lsl #12
+
+  ret
+.Lfunc_end0:
+  .size  main, .Lfunc_end0-main

test/MC/AArch64/basic-a64-diagnostics.s

 // CHECK-ERROR: error: expected #imm after shift specifier
 // CHECK-ERROR:         add sp, x5, x7, lsl
 // CHECK-ERROR:                             ^
 
 //------------------------------------------------------------------------------
 // Add/sub (immediate)
 //------------------------------------------------------------------------------
 
-// Out of range immediates: < 0 or more than 12 bits
-        add w4, w5, #-1
-        add w5, w6, #0x1000
-        add w4, w5, #-1, lsl #12
+// Out of range immediates: wrong instruction to flip or more than 12 bits
+        add w5, w6, #0x1001
         add w5, w6, #0x1000, lsl #12
-// CHECK-ERROR: error: expected compatible register, symbol or integer in range [0, 4095]
-// CHECK-ERROR-NEXT:         add w4, w5, #-1
-// CHECK-ERROR-NEXT:                     ^
-// CHECK-ERROR-AARCH64-NEXT: error: expected compatible register, symbol or integer in range [0, 4095]
-// CHECK-ERROR-AARCH64-NEXT:         add w5, w6, #0x1000
-// CHECK-ERROR-AARCH64-NEXT:                     ^
+        add w5, w6, #-4096
+        adds w5, w6, #-1
 // CHECK-ERROR-NEXT: error: expected compatible register, symbol or integer in range [0, 4095]
-// CHECK-ERROR-NEXT:         add w4, w5, #-1, lsl #12
+// CHECK-ERROR-NEXT:         add w5, w6, #0x1001
 // CHECK-ERROR-NEXT:                     ^
 // CHECK-ERROR-NEXT: error: expected compatible register, symbol or integer in range [0, 4095]
 // CHECK-ERROR-NEXT:         add w5, w6, #0x1000, lsl #12
 // CHECK-ERROR-NEXT:                     ^
+// CHECK-ERROR-NEXT: error: expected compatible register, symbol or integer in range [0, 4095]
+// CHECK-ERROR-NEXT:         add w5, w6, #-4096
+// CHECK-ERROR-NEXT:                     ^
+// CHECK-ERROR-NEXT: error: negative immediate only in ADD/SUB up to -4095
+// CHECK-ERROR-NEXT:         adds w5, w6, #-1
+// CHECK-ERROR-NEXT:                      ^
 
 // Only lsl #0 and lsl #12 are allowed
         add w2, w3, #0x1, lsl #1
         add w5, w17, #0xfff, lsl #13
         add w17, w20, #0x1000, lsl #12
         sub xsp, x34, #0x100, lsl #-1
 // CHECK-ERROR: error: expected compatible register, symbol or integer in range [0, 4095]
 // CHECK-ERROR-NEXT:         add w2, w3, #0x1, lsl #1