[ARM64] Re-work parsing of ADD/SUB shifted immediate operands

[bsmith]

Summary

The parsing of ADD/SUB shifted immediates needs to be done explicitly so that better diagnostics can be emitted, as a side effect this also removes some of the hacks in the current method of handling this operand type.

Additionally remove manual CMP aliasing to ADD/SUB and use InstAlias instead.

There is no testcase for this fix since it is part of porting over MC/AArch64/basic-a64-diagnostics.s, which will be enabled for ARM64 once everything is fixed in it.

[t.p.northover]

Hi Bradley,

I've got a few more comments on this one...

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

581–583

I don't think these are correct here. They're for unpacking a binary encoded shift, but tryParseAddSubImm gives no indication that's what it's creating (& the name ShiftAmount would be misleading if it did).

2309–2310

Given its name and current purpose, this function seems more generic than necessary. (E.g. accepting arbitrary shift amounts when only 12 is ever valid).

Do you intend to use it for other cases, or should it be simplified?

[bsmith]

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

581–583

Good catch, this is wrong since it's just the amount not the special encoded amount. I just so happened to get away with this since LSL = 0..

2309–2310

No it was only ever intended for this purpose. The 0/12 only logic is in the predicate function, but if you think it makes more sense to have it here instead then I can change this.

[bsmith]

New patch that removes the incorrect use of ARM64_AM shift encodings in isAddSubImm and tightens up the shift parsing in tryParseAddSubImm to disallow garbage shifts. As for simplifying tryParseAddSubImm, I'm not convinced that only allowing 12/0 there is going to actually simplify anything, but rather just move code from isAddSubImm.

[bsmith]

(Minor fix to previous patch)

[t.p.northover]

Hi Bradley,

Fair enough. The difference is marginal either way on that one. I did spot another nit-pick though:

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

588

I think this might be undefined behaviour if we're actually in the isImm case: accessing a union via a member that wasn't most recently written, or whatever.

If it was me I'd handle isShiftedImm entirely within its if block and reserve the rest of the function for the non-shifted case.

[bsmith]

Fixup potential union related undefined behavior in isAddSubImm. Unfortunately this can't reasonably be done quite as suggested without code duplication since the classifySymbolRef section needs to work for both the Imm and ShiftedImm cases.

[t.p.northover]

Hi Bradley,

I think this looks good now. Thanks for going through so many revisions!

Tim.

lib/Target/ARM64/ARM64InstrFormats.td

 }
 
 // Shifter operand for arithmetic register shifted encodings.
 def ArithmeticShifterOperand : AsmOperandClass {
   let SuperClasses = [ShifterOperand];
   let Name = "ArithmeticShifter";
 }
 
-// Shifter operand for arithmetic shifted encodings for ADD/SUB instructions.
-def AddSubShifterOperand : AsmOperandClass {
-  let SuperClasses = [ArithmeticShifterOperand];
-  let Name = "AddSubShifter";
-}
-
 // Shifter operand for logical vector 128/64-bit shifted encodings.
 def LogicalVecShifterOperand : AsmOperandClass {
   let SuperClasses = [ShifterOperand];
   let Name = "LogicalVecShifter";
 }
 def LogicalVecHalfWordShifterOperand : AsmOperandClass {
   let SuperClasses = [LogicalVecShifterOperand];
   let Name = "LogicalVecHalfWordShifter";

 // A vector move shifter operand:
 //  {0} - imm1: #8 or #16
 def move_vec_shift : Operand<i32> {
   let PrintMethod = "printShifter";
   let EncoderMethod = "getMoveVecShifterOpValue";
   let ParserMatchClass = MoveVecShifterOperand;
 }
 
+def AddSubImmOperand : AsmOperandClass {
+  let Name = "AddSubImm";
+  let ParserMethod = "tryParseAddSubImm";
+  let DiagnosticType = "AddSubSecondSource";
+}
 // An ADD/SUB immediate shifter operand:
+//  second operand:
 //  {7-6} - shift type: 00 = lsl
 //  {5-0} - imm6: #0 or #12
-def addsub_shift : Operand<i32> {
-  let ParserMatchClass = AddSubShifterOperand;
-}
-
 class addsub_shifted_imm<ValueType Ty>
     : Operand<Ty>, ComplexPattern<Ty, 2, "SelectArithImmed", [imm]> {
   let PrintMethod = "printAddSubImm";
   let EncoderMethod = "getAddSubImmOpValue";
-  let MIOperandInfo = (ops i32imm, addsub_shift);
+  let ParserMatchClass = AddSubImmOperand;
+  let MIOperandInfo = (ops i32imm, i32imm);
 }
 
 def addsub_shifted_imm32 : addsub_shifted_imm<i32>;
 def addsub_shifted_imm64 : addsub_shifted_imm<i64>;
 
 class neg_addsub_shifted_imm<ValueType Ty>
     : Operand<Ty>, ComplexPattern<Ty, 2, "SelectNegArithImmed", [imm]> {
   let PrintMethod = "printAddSubImm";
   let EncoderMethod = "getAddSubImmOpValue";
-  let MIOperandInfo = (ops i32imm, addsub_shift);
+  let ParserMatchClass = AddSubImmOperand;
+  let MIOperandInfo = (ops i32imm, i32imm);
 }
 
 def neg_addsub_shifted_imm32 : neg_addsub_shifted_imm<i32>;
 def neg_addsub_shifted_imm64 : neg_addsub_shifted_imm<i64>;
 
 // An extend operand:
 //  {5-3} - extend type
 //  {2-0} - imm3
multiclass AddSub<bit isSub, string mnemonic,
   // above matching first in the case when SP is not used.
   def : AddSubRegAlias<mnemonic, !cast<Instruction>(NAME#"Wrx"),
                        GPR32sp, GPR32sp, GPR32, 16>; // UXTW #0
   def : AddSubRegAlias<mnemonic,
                        !cast<Instruction>(NAME#"Xrx64"),
                        GPR64sp, GPR64sp, GPR64, 24>; // UXTX #0
 }
 
-multiclass AddSubS<bit isSub, string mnemonic, SDNode OpNode> {
+multiclass AddSubS<bit isSub, string mnemonic, SDNode OpNode, string cmp> {
   let isCompare = 1, Defs = [NZCV] in {
   // Add/Subtract immediate
   def Wri  : BaseAddSubImm<isSub, 1, GPR32, GPR32sp, addsub_shifted_imm32,
                            mnemonic, OpNode> {
     let Inst{31} = 0;
   }
   def Xri  : BaseAddSubImm<isSub, 1, GPR64, GPR64sp, addsub_shifted_imm64,
                            mnemonic, OpNode> {
multiclass AddSubS<bit isSub, string mnemonic, SDNode OpNode, string cmp> {
   def Xrx64 : BaseAddSubEReg64<isSub, 1, GPR64, GPR64sp, GPR64,
                                arith_extendlsl64, mnemonic> {
     // UXTX and SXTX only.
     let Inst{14-13} = 0b11;
     let Inst{31} = 1;
   }
   } // Defs = [NZCV]
 
+  // Compare aliases
+  def : InstAlias<cmp#" $src, $imm", (!cast<Instruction>(NAME#"Wri")
+                  WZR, GPR32sp:$src, addsub_shifted_imm32:$imm)>;
+  def : InstAlias<cmp#" $src, $imm", (!cast<Instruction>(NAME#"Xri")
+                  XZR, GPR64sp:$src, addsub_shifted_imm64:$imm)>;
+  def : InstAlias<cmp#" $src1, $src2, $sh", (!cast<Instruction>(NAME#"Wrx")
+                  WZR, GPR32sp:$src1, GPR32:$src2, arith_extend:$sh)>;
+  def : InstAlias<cmp#" $src1, $src2, $sh", (!cast<Instruction>(NAME#"Xrx")
+                  XZR, GPR64sp:$src1, GPR32:$src2, arith_extend:$sh)>;
+  def : InstAlias<cmp#" $src1, $src2, $sh", (!cast<Instruction>(NAME#"Xrx64")
+                  XZR, GPR64sp:$src1, GPR64:$src2, arith_extendlsl64:$sh)>;
+  def : InstAlias<cmp#" $src1, $src2, $sh", (!cast<Instruction>(NAME#"Wrs")
+                  WZR, GPR32:$src1, GPR32:$src2, arith_shift:$sh)>;
+  def : InstAlias<cmp#" $src1, $src2, $sh", (!cast<Instruction>(NAME#"Xrs")
+                  XZR, GPR64:$src1, GPR64:$src2, arith_shift:$sh)>;
+
+  // Compare shorthands
+  def : InstAlias<cmp#" $src1, $src2", (!cast<Instruction>(NAME#"Wrs")
+                  WZR, GPR32:$src1, GPR32:$src2, 0)>;
+  def : InstAlias<cmp#" $src1, $src2", (!cast<Instruction>(NAME#"Xrs")
+                  XZR, GPR64:$src1, GPR64:$src2, 0)>;
+
   // Register/register aliases with no shift when SP is not used.
   def : AddSubRegAlias<mnemonic, !cast<Instruction>(NAME#"Wrs"),
                        GPR32, GPR32, GPR32, 0>;
   def : AddSubRegAlias<mnemonic, !cast<Instruction>(NAME#"Xrs"),
                        GPR64, GPR64, GPR64, 0>;
 
   // Register/register aliases with no shift when the first source register
   // is SP.  This relies on the shifted register aliases above matching first

lib/Target/ARM64/ARM64InstrInfo.td

 def : InstAlias<"ngc $dst, $src",  (SBCXr  GPR64:$dst, XZR, GPR64:$src)>;
 def : InstAlias<"ngcs $dst, $src", (SBCSWr GPR32:$dst, WZR, GPR32:$src)>;
 def : InstAlias<"ngcs $dst, $src", (SBCSXr GPR64:$dst, XZR, GPR64:$src)>;
 
 // Add/subtract
 defm ADD : AddSub<0, "add", add>;
 defm SUB : AddSub<1, "sub">;
 
-defm ADDS : AddSubS<0, "adds", ARM64add_flag>;
-defm SUBS : AddSubS<1, "subs", ARM64sub_flag>;
+defm ADDS : AddSubS<0, "adds", ARM64add_flag, "cmn">;
+defm SUBS : AddSubS<1, "subs", ARM64sub_flag, "cmp">;
 
 // Use SUBS instead of SUB to enable CSE between SUBS and SUB.
 def : Pat<(sub GPR32sp:$Rn, addsub_shifted_imm32:$imm),
           (SUBSWri GPR32sp:$Rn, addsub_shifted_imm32:$imm)>;
 def : Pat<(sub GPR64sp:$Rn, addsub_shifted_imm64:$imm),
           (SUBSXri GPR64sp:$Rn, addsub_shifted_imm64:$imm)>;
 def : Pat<(sub GPR32:$Rn, GPR32:$Rm),
           (SUBSWrr GPR32:$Rn, GPR32:$Rm)>;

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

#include "ARM64GenAsmMatcher.inc"
   OperandMatchResultTy tryParseBarrierOperand(OperandVector &Operands);
   OperandMatchResultTy tryParseMRSSystemRegister(OperandVector &Operands);
   OperandMatchResultTy tryParseSysReg(OperandVector &Operands);
   OperandMatchResultTy tryParseSysCROperand(OperandVector &Operands);
   OperandMatchResultTy tryParsePrefetch(OperandVector &Operands);
   OperandMatchResultTy tryParseAdrpLabel(OperandVector &Operands);
   OperandMatchResultTy tryParseAdrLabel(OperandVector &Operands);
   OperandMatchResultTy tryParseFPImm(OperandVector &Operands);
+  OperandMatchResultTy tryParseAddSubImm(OperandVector &Operands);
   bool tryParseVectorRegister(OperandVector &Operands);
 
 public:
   enum ARM64MatchResultTy {
     Match_InvalidSuffix = FIRST_TARGET_MATCH_RESULT_TY,
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "ARM64GenAsmMatcher.inc"
   };
public:
   enum MemIdxKindTy {
     ImmediateOffset, // pre-indexed, no writeback
     RegisterOffset   // register offset, with optional extend
   };
 
 private:
   enum KindTy {
     k_Immediate,
+    k_ShiftedImm,
     k_Memory,
     k_Register,
     k_VectorList,
     k_VectorIndex,
     k_Token,
     k_SysReg,
     k_SysCR,
     k_Prefetch,
private:
   struct VectorIndexOp {
     unsigned Val;
   };
 
   struct ImmOp {
     const MCExpr *Val;
   };
 
+  struct ShiftedImmOp {
+    const MCExpr *Val;
+    unsigned ShiftAmount;
+  };
+
   struct FPImmOp {
     unsigned Val; // Encoded 8-bit representation.
   };
 
   struct BarrierOp {
     unsigned Val; // Not the enum since not all values have names.
   };
 
private:
   };
 
   union {
     struct TokOp Tok;
     struct RegOp Reg;
     struct VectorListOp VectorList;
     struct VectorIndexOp VectorIndex;
     struct ImmOp Imm;
+    struct ShiftedImmOp ShiftedImm;
     struct FPImmOp FPImm;
     struct BarrierOp Barrier;
     struct SysRegOp SysReg;
     struct SysCRImmOp SysCRImm;
     struct PrefetchOp Prefetch;
     struct ShifterOp Shifter;
     struct ExtendOp Extend;
     struct MemOp Mem;
ARM64Operand(const ARM64Operand &o) : MCParsedAsmOperand(), Ctx(o.Ctx) {
     EndLoc = o.EndLoc;
     switch (Kind) {
     case k_Token:
       Tok = o.Tok;
       break;
     case k_Immediate:
       Imm = o.Imm;
       break;
+    case k_ShiftedImm:
+      ShiftedImm = o.ShiftedImm;
+      break;
     case k_FPImm:
       FPImm = o.FPImm;
       break;
     case k_Barrier:
       Barrier = o.Barrier;
       break;
     case k_Register:
       Reg = o.Reg;
bool isTokenSuffix() const {
     return Tok.IsSuffix;
   }
 
   const MCExpr *getImm() const {
     assert(Kind == k_Immediate && "Invalid access!");
     return Imm.Val;
   }
 
+  const MCExpr *getShiftedImmVal() const {
+    assert(Kind == k_ShiftedImm && "Invalid access!");
+    return ShiftedImm.Val;
+  }
+
+  unsigned getShiftedImmShift() const {
+    assert(Kind == k_ShiftedImm && "Invalid access!");
+    return ShiftedImm.ShiftAmount;
+  }
+
   unsigned getFPImm() const {
     assert(Kind == k_FPImm && "Invalid access!");
     return FPImm.Val;
   }
 
   unsigned getBarrier() const {
     assert(Kind == k_Barrier && "Invalid access!");
     return Barrier.Val;
public:
   bool isLogicalImm64() const {
     if (!isImm())
       return false;
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
     if (!MCE)
       return false;
     return ARM64_AM::isLogicalImmediate(MCE->getValue(), 64);
   }
+  bool isShiftedImm() const { return Kind == k_ShiftedImm; }
+  bool isAddSubImm() const {
+    if (!isShiftedImm() && !isImm())
+      return false;
+
+    const MCExpr *Expr;
+
+    // An ADD/SUB shifter is either 'lsl #0' or 'lsl #12'.
+    if (isShiftedImm()) {
+      unsigned Shift = ShiftedImm.ShiftAmount;
+      Expr = ShiftedImm.Val;
+      if (Shift != 0 && Shift != 12)
+        return false;
+    } else {
+      Expr = getImm();
+    }
+
+    ARM64MCExpr::VariantKind ELFRefKind;
+    MCSymbolRefExpr::VariantKind DarwinRefKind;
+    int64_t Addend;
+    if (ARM64AsmParser::classifySymbolRef(Expr, ELFRefKind,
+                                          DarwinRefKind, Addend)) {
+      return DarwinRefKind == MCSymbolRefExpr::VK_PAGEOFF
+          || DarwinRefKind == MCSymbolRefExpr::VK_TLVPPAGEOFF
+          || (DarwinRefKind == MCSymbolRefExpr::VK_GOTPAGEOFF && Addend == 0)
+          || ELFRefKind == ARM64MCExpr::VK_LO12
+          || ELFRefKind == ARM64MCExpr::VK_DTPREL_HI12
+          || ELFRefKind == ARM64MCExpr::VK_DTPREL_LO12
+          || ELFRefKind == ARM64MCExpr::VK_DTPREL_LO12_NC
+          || ELFRefKind == ARM64MCExpr::VK_TPREL_HI12
+          || ELFRefKind == ARM64MCExpr::VK_TPREL_LO12
+          || ELFRefKind == ARM64MCExpr::VK_TPREL_LO12_NC
+          || ELFRefKind == ARM64MCExpr::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;
+  }
   bool isSIMDImmType10() const {
     if (!isImm())
       return false;
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
     if (!MCE)
       return false;
     return ARM64_AM::isAdvSIMDModImmType10(MCE->getValue());
   }
bool isMovImm64Shifter() const {
     // A MOVi shifter is LSL of 0 or 16.
     ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(Shifter.Val);
     if (ST != ARM64_AM::LSL)
       return false;
     uint64_t Val = ARM64_AM::getShiftValue(Shifter.Val);
     return (Val == 0 || Val == 16 || Val == 32 || Val == 48);
   }
 
-  bool isAddSubShifter() const {
-    if (!isShifter())
-      return false;
-
-    // An ADD/SUB shifter is either 'lsl #0' or 'lsl #12'.
-    unsigned Val = Shifter.Val;
-    return ARM64_AM::getShiftType(Val) == ARM64_AM::LSL &&
-           (ARM64_AM::getShiftValue(Val) == 0 ||
-            ARM64_AM::getShiftValue(Val) == 12);
-  }
-
   bool isLogicalVecShifter() const {
     if (!isShifter())
       return false;
 
     // A logical vector shifter is a left shift by 0, 8, 16, or 24.
     unsigned Val = Shifter.Val;
     unsigned Shift = ARM64_AM::getShiftValue(Val);
     return ARM64_AM::getShiftType(Val) == ARM64_AM::LSL &&
public:
   void addImmOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     // If this is a pageoff symrefexpr with an addend, adjust the addend
     // to be only the page-offset portion. Otherwise, just add the expr
     // as-is.
     addExpr(Inst, getImm());
   }
 
+  void addAddSubImmOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 2 && "Invalid number of operands!");
+    if (isShiftedImm()) {
+      addExpr(Inst, getShiftedImmVal());
+      Inst.addOperand(MCOperand::CreateImm(getShiftedImmShift()));
+    } else {
+      addExpr(Inst, getImm());
+      Inst.addOperand(MCOperand::CreateImm(0));
+    }
+  }
+
   void addAdrpLabelOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(getImm());
     if (!MCE)
       addExpr(Inst, getImm());
     else
       Inst.addOperand(MCOperand::CreateImm(MCE->getValue() >> 12));
   }
void addMovImm32ShifterOperands(MCInst &Inst, unsigned N) const {
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
   }
 
   void addMovImm64ShifterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
   }
 
-  void addAddSubShifterOperands(MCInst &Inst, unsigned N) const {
-    assert(N == 1 && "Invalid number of operands!");
-    Inst.addOperand(MCOperand::CreateImm(getShifter()));
-  }
-
   void addLogicalVecShifterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
   }
 
   void addLogicalVecHalfWordShifterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
static ARM64Operand *CreateImm(const MCExpr *Val, SMLoc S, SMLoc E,
                                  MCContext &Ctx) {
     ARM64Operand *Op = new ARM64Operand(k_Immediate, Ctx);
     Op->Imm.Val = Val;
     Op->StartLoc = S;
     Op->EndLoc = E;
     return Op;
   }
 
+  static ARM64Operand *CreateShiftedImm(const MCExpr *Val, unsigned ShiftAmount,
+                                        SMLoc S, SMLoc E, MCContext &Ctx) {
+    ARM64Operand *Op = new ARM64Operand(k_ShiftedImm, Ctx);
+    Op->ShiftedImm .Val = Val;
+    Op->ShiftedImm.ShiftAmount = ShiftAmount;
+    Op->StartLoc = S;
+    Op->EndLoc = E;
+    return Op;
+  }
+
   static ARM64Operand *CreateFPImm(unsigned Val, SMLoc S, MCContext &Ctx) {
     ARM64Operand *Op = new ARM64Operand(k_FPImm, Ctx);
     Op->FPImm.Val = Val;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
if (Valid)
       OS << "<barrier " << Name << ">";
     else
       OS << "<barrier invalid #" << getBarrier() << ">";
     break;
   }
   case k_Immediate:
     getImm()->print(OS);
     break;
+  case k_ShiftedImm: {
+    unsigned Shift = getShiftedImmShift();
+    OS << "<shiftedimm ";
+    getShiftedImmVal()->print(OS);
+    OS << ", " << ARM64_AM::getShiftName(ARM64_AM::getShiftType(Shift)) << " #"
+       << ARM64_AM::getShiftValue(Shift) << ">";
+    break;
+  }
   case k_Memory:
     OS << "<memory>";
     break;
   case k_Register:
     OS << "<register " << getReg() << ">";
     break;
   case k_VectorList: {
     OS << "<vectorlist ";
ARM64AsmParser::tryParseFPImm(OperandVector &Operands) {
 
   if (!Hash)
     return MatchOperand_NoMatch;
 
   TokError("invalid floating point immediate");
   return MatchOperand_ParseFail;
 }
 
+/// tryParseAddSubImm - Parse ADD/SUB shifted immediate operand
+ARM64AsmParser::OperandMatchResultTy
+ARM64AsmParser::tryParseAddSubImm(OperandVector &Operands) {
+  SMLoc S = getLoc();
+
+  if (Parser.getTok().is(AsmToken::Hash))
+    Parser.Lex(); // Eat '#'
+  else if (Parser.getTok().isNot(AsmToken::Integer))
+    // Operand should start from # or should be integer, emit error otherwise.
+    return MatchOperand_NoMatch;
+
+  const MCExpr *Imm;
+  if (parseSymbolicImmVal(Imm))
+    return MatchOperand_ParseFail;
+  else if (Parser.getTok().isNot(AsmToken::Comma)) {
+    uint64_t ShiftAmount = 0;
+    const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(Imm);
+    if (MCE) {
+      int64_t Val = MCE->getValue();
+      if (Val > 0xfff && (Val & 0xfff) == 0) {
+        Imm = MCConstantExpr::Create(Val >> 12, getContext());
+        ShiftAmount = 12;
+      }
+    }
+    SMLoc E = Parser.getTok().getLoc();
+    Operands.push_back(ARM64Operand::CreateShiftedImm(Imm, ShiftAmount, S, E,
+                                                      getContext()));
+    return MatchOperand_Success;
+  }
+
+  // Eat ','
+  Parser.Lex();
+
+  // The optional operand must be "lsl #N" where N is non-negative.
+  if (!Parser.getTok().is(AsmToken::Identifier) ||
+      !Parser.getTok().getIdentifier().equals_lower("lsl")) {
+    Error(Parser.getTok().getLoc(), "only 'lsl #+N' valid after immediate");
+    return MatchOperand_ParseFail;
+  }
+
+  // Eat 'lsl'
+  Parser.Lex();
+
+  if (Parser.getTok().is(AsmToken::Hash)) {
+    Parser.Lex();
+  }
+
+  if (Parser.getTok().isNot(AsmToken::Integer)) {
+    Error(Parser.getTok().getLoc(), "only 'lsl #+N' valid after immediate");
+    return MatchOperand_ParseFail;
+  }
+
+  int64_t ShiftAmount = Parser.getTok().getIntVal();
+
+  if (ShiftAmount < 0) {
+    Error(Parser.getTok().getLoc(), "positive shift amount required");
+    return MatchOperand_ParseFail;
+  }
+  Parser.Lex(); // Eat the number
+
+  SMLoc E = Parser.getTok().getLoc();
+  Operands.push_back(ARM64Operand::CreateShiftedImm(Imm, ShiftAmount,
+                                                    S, E, getContext()));
+  return MatchOperand_Success;
+}
+
 /// parseCondCodeString - Parse a Condition Code string.
 unsigned ARM64AsmParser::parseCondCodeString(StringRef Cond) {
   unsigned CC = StringSwitch<unsigned>(Cond.lower())
                     .Case("eq", ARM64CC::EQ)
                     .Case("ne", ARM64CC::NE)
                     .Case("cs", ARM64CC::HS)
                     .Case("hs", ARM64CC::HS)
                     .Case("cc", ARM64CC::LO)
if (ShOp == ARM64_AM::InvalidShift)
     return MatchOperand_NoMatch;
 
   SMLoc S = Tok.getLoc();
   Parser.Lex();
 
   // We expect a number here.
   bool Hash = getLexer().is(AsmToken::Hash);
   if (!Hash && getLexer().isNot(AsmToken::Integer)) {
-    TokError("immediate value expected for shifter operand");
+    TokError("expected #imm after shift specifier");
     return MatchOperand_ParseFail;
   }
 
   if (Hash)
     Parser.Lex(); // Eat the '#'.
 
   // Make sure we do actually have a number
   if (!Parser.getTok().is(AsmToken::Integer)) {
     Error(Parser.getTok().getLoc(),
           "expected integer shift amount");
     return MatchOperand_ParseFail;
   }
 
   SMLoc ExprLoc = getLoc();
   const MCExpr *ImmVal;
   if (getParser().parseExpression(ImmVal))
     return MatchOperand_ParseFail;
 
   const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(ImmVal);
   if (!MCE) {
-    TokError("immediate value expected for shifter operand");
+    TokError("expected #imm after shift specifier");
     return MatchOperand_ParseFail;
   }
 
   if ((MCE->getValue() & 0x3f) != MCE->getValue()) {
     Error(ExprLoc, "immediate value too large for shifter operand");
     return MatchOperand_ParseFail;
   }
 
bool ARM64AsmParser::validateInstruction(MCInst &Inst,
   case ARM64::ADDSWri:
   case ARM64::ADDSXri:
   case ARM64::ADDWri:
   case ARM64::ADDXri:
   case ARM64::SUBSWri:
   case ARM64::SUBSXri:
   case ARM64::SUBWri:
   case ARM64::SUBXri: {
-    if (!Inst.getOperand(3).isImm())
-      return Error(Loc[3], "immediate value expected");
-    int64_t shifter = Inst.getOperand(3).getImm();
-    if (shifter != 0 && shifter != 12)
-      return Error(Loc[3], "shift value out of range");
-    // The imm12 operand can be an expression. Validate that it's legit.
-    // FIXME: We really, really want to allow arbitrary expressions here
-    // and resolve the value and validate the result at fixup time, but
-    // that's hard as we have long since lost any source information we
-    // need to generate good diagnostics by that point.
-    if ((Inst.getOpcode() == ARM64::ADDXri ||
-         Inst.getOpcode() == ARM64::ADDWri) &&
-        Inst.getOperand(2).isExpr()) {
+    // Annoyingly we can't do this in the isAddSubImm predicate, so there is
+    // some slight duplication here.
+    if (Inst.getOperand(2).isExpr()) {
       const MCExpr *Expr = Inst.getOperand(2).getExpr();
       ARM64MCExpr::VariantKind ELFRefKind;
       MCSymbolRefExpr::VariantKind DarwinRefKind;
       int64_t Addend;
       if (!classifySymbolRef(Expr, ELFRefKind, DarwinRefKind, Addend)) {
         return Error(Loc[2], "invalid immediate expression");
       }
 
-      // Note that we don't range-check the addend. It's adjusted modulo page
-      // size when converted, so there is no "out of range" condition when using
-      // @pageoff. Any validity checking for the value was done in the is*()
-      // predicate function.
+      // Only allow these with ADDXri.
       if ((DarwinRefKind == MCSymbolRefExpr::VK_PAGEOFF ||
-           DarwinRefKind == MCSymbolRefExpr::VK_TLVPPAGEOFF) &&
           Inst.getOpcode() == ARM64::ADDXri)
         return false;
-      if (ELFRefKind == ARM64MCExpr::VK_LO12 ||
+
+      // Only allow these with ADDXri/ADDWri
+      if ((ELFRefKind == ARM64MCExpr::VK_LO12 ||
           ELFRefKind == ARM64MCExpr::VK_DTPREL_HI12 ||
           ELFRefKind == ARM64MCExpr::VK_DTPREL_LO12 ||
           ELFRefKind == ARM64MCExpr::VK_DTPREL_LO12_NC ||
           ELFRefKind == ARM64MCExpr::VK_TPREL_HI12 ||
           ELFRefKind == ARM64MCExpr::VK_TPREL_LO12 ||
           ELFRefKind == ARM64MCExpr::VK_TPREL_LO12_NC ||
-          ELFRefKind == ARM64MCExpr::VK_TLSDESC_LO12) {
+          ELFRefKind == ARM64MCExpr::VK_TLSDESC_LO12) &&
+          (Inst.getOpcode() == ARM64::ADDXri ||
+          Inst.getOpcode() == ARM64::ADDWri))
         return false;
-      } else if (DarwinRefKind == MCSymbolRefExpr::VK_GOTPAGEOFF) {
-        // @gotpageoff can only be used directly, not with an addend.
-        return Addend != 0;
-      }
 
-      // Otherwise, we're not sure, so don't allow it for now.
+      // Don't allow expressions in the immediate field otherwise
       return Error(Loc[2], "invalid immediate expression");
     }
-
-    // If it's anything but an immediate, it's not legit.
-    if (!Inst.getOperand(2).isImm())
-      return Error(Loc[2], "invalid immediate expression");
-    int64_t imm = Inst.getOperand(2).getImm();
-    if (imm > 4095 || imm < 0)
-      return Error(Loc[2], "immediate value out of range");
     return false;
   }
   case ARM64::LDRBpre:
   case ARM64::LDRHpre:
   case ARM64::LDRSBWpre:
   case ARM64::LDRSBXpre:
   case ARM64::LDRSHWpre:
   case ARM64::LDRSHXpre:

 static void rewriteMOVRSP(ARM64AsmParser::OperandVector &Operands,
                         MCContext &Context) {
   ARM64Operand *Op = static_cast<ARM64Operand *>(Operands[0]);
   ARM64Operand *Op2 = static_cast<ARM64Operand *>(Operands[2]);
   Operands[0] =
     ARM64Operand::CreateToken("add", false, Op->getStartLoc(), Context);
 
   const MCExpr *Imm = MCConstantExpr::Create(0, Context);
-  Operands.push_back(ARM64Operand::CreateImm(Imm, Op2->getStartLoc(),
+  Operands.push_back(ARM64Operand::CreateShiftedImm(Imm, 0, Op2->getStartLoc(),
                                              Op2->getEndLoc(), Context));
-  Operands.push_back(ARM64Operand::CreateShifter(
-      ARM64_AM::LSL, 0, Op2->getStartLoc(), Op2->getEndLoc(), Context));
 
   delete Op;
 }
 
 static void rewriteMOVR(ARM64AsmParser::OperandVector &Operands,
                         MCContext &Context) {
   ARM64Operand *Op = static_cast<ARM64Operand *>(Operands[0]);
   ARM64Operand *Op2 = static_cast<ARM64Operand *>(Operands[2]);
bool ARM64AsmParser::showMatchError(SMLoc Loc, unsigned ErrCode) {
   case Match_InvalidSuffix:
     return Error(Loc, "invalid type suffix for instruction");
   case Match_AddSubRegExtendSmall:
     return Error(Loc,
       "expected '[su]xt[bhw]' or 'lsl' with optional integer in range [0, 4]");
   case Match_AddSubRegExtendLarge:
     return Error(Loc,
       "expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]");
+  case Match_AddSubSecondSource:
+    return Error(Loc,
+      "expected compatible register, symbol or integer in range [0, 4095]");
   case Match_InvalidMemoryIndexedSImm9:
     return Error(Loc, "index must be an integer in range [-256, 255].");
   case Match_InvalidMemoryIndexed32SImm7:
     return Error(Loc, "index must be a multiple of 4 in range [-256, 252].");
   case Match_InvalidMemoryIndexed64SImm7:
     return Error(Loc, "index must be a multiple of 8 in range [-512, 504].");
   case Match_InvalidMemoryIndexed128SImm7:
     return Error(Loc, "index must be a multiple of 16 in range [-1024, 1008].");
bool ARM64AsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                              MCStreamer &Out,
                                              unsigned &ErrorInfo,
                                              bool MatchingInlineAsm) {
   assert(!Operands.empty() && "Unexpect empty operand list!");
   ARM64Operand *Op = static_cast<ARM64Operand *>(Operands[0]);
   assert(Op->isToken() && "Leading operand should always be a mnemonic!");
 
   StringRef Tok = Op->getToken();
-  // Translate CMN/CMP pseudos to ADDS/SUBS with zero register destination.
-  // This needs to be done before the special handling of ADD/SUB immediates.
-  if (Tok == "cmp" || Tok == "cmn") {
-    // Replace the opcode with either ADDS or SUBS.
-    const char *Repl = StringSwitch<const char *>(Tok)
-                           .Case("cmp", "subs")
-                           .Case("cmn", "adds")
-                           .Default(nullptr);
-    assert(Repl && "Unknown compare instruction");
-    delete Operands[0];
-    Operands[0] = ARM64Operand::CreateToken(Repl, false, IDLoc, getContext());
-
-    // Insert WZR or XZR as destination operand.
-    ARM64Operand *RegOp = static_cast<ARM64Operand *>(Operands[1]);
-    unsigned ZeroReg;
-    if (RegOp->isReg() &&
-        ARM64MCRegisterClasses[ARM64::GPR32allRegClassID].contains(
-            RegOp->getReg()))
-      ZeroReg = ARM64::WZR;
-    else
-      ZeroReg = ARM64::XZR;
-    Operands.insert(
-        Operands.begin() + 1,
-        ARM64Operand::CreateReg(ZeroReg, false, IDLoc, IDLoc, getContext()));
-    // Update since we modified it above.
-    ARM64Operand *Op = static_cast<ARM64Operand *>(Operands[0]);
-    Tok = Op->getToken();
-  }
-
   unsigned NumOperands = Operands.size();
 
   if (Tok == "mov" && NumOperands == 3) {
     // The MOV mnemomic is aliased to movn/movz, depending on the value of
     // the immediate being instantiated.
     // FIXME: Catching this here is a total hack, and we should use tblgen
     // support to implement this instead as soon as it is available.
 
if (Op2->isImm()) {
            ARM64MCRegisterClasses[ARM64::GPR32allRegClassID].contains(Reg2)) ||
           (Reg2 == ARM64::WSP &&
            ARM64MCRegisterClasses[ARM64::GPR32allRegClassID].contains(Reg1)))
         rewriteMOVRSP(Operands, getContext());
       else
         rewriteMOVR(Operands, getContext());
     }
   } else if (NumOperands == 4) {
-    if (Tok == "add" || Tok == "adds" || Tok == "sub" || Tok == "subs") {
-      // Handle the uimm24 immediate form, where the shift is not specified.
-      ARM64Operand *Op3 = static_cast<ARM64Operand *>(Operands[3]);
-      if (Op3->isImm()) {
-        if (const MCConstantExpr *CE =
-                dyn_cast<MCConstantExpr>(Op3->getImm())) {
-          uint64_t Val = CE->getValue();
-          if (Val >= (1 << 24)) {
-            Error(IDLoc, "immediate value is too large");
-            return true;
-          }
-          if (Val < (1 << 12)) {
-            Operands.push_back(ARM64Operand::CreateShifter(
-                ARM64_AM::LSL, 0, IDLoc, IDLoc, getContext()));
-          } else if ((Val & 0xfff) == 0) {
-            delete Operands[3];
-            CE = MCConstantExpr::Create(Val >> 12, getContext());
-            Operands[3] =
-                ARM64Operand::CreateImm(CE, IDLoc, IDLoc, getContext());
-            Operands.push_back(ARM64Operand::CreateShifter(
-                ARM64_AM::LSL, 12, IDLoc, IDLoc, getContext()));
-          } else {
-            Error(IDLoc, "immediate value is too large");
-            return true;
-          }
-        } else {
-          Operands.push_back(ARM64Operand::CreateShifter(
-              ARM64_AM::LSL, 0, IDLoc, IDLoc, getContext()));
-        }
-      }
-
-      // FIXME: Horible hack to handle the LSL -> UBFM alias.
-    } else if (NumOperands == 4 && Tok == "lsl") {
+    if (NumOperands == 4 && Tok == "lsl") {
       ARM64Operand *Op2 = static_cast<ARM64Operand *>(Operands[2]);
       ARM64Operand *Op3 = static_cast<ARM64Operand *>(Operands[3]);
       if (Op2->isReg() && Op3->isImm()) {
         const MCConstantExpr *Op3CE = dyn_cast<MCConstantExpr>(Op3->getImm());
         if (Op3CE) {
           uint64_t Op3Val = Op3CE->getValue();
           uint64_t NewOp3Val = 0;
           uint64_t NewOp4Val = 0;
case Match_InvalidMemoryIndexed128:
     // If there is a '!' after the memory operand that failed, we really
     // want the diagnostic for the pre-indexed instruction variant instead.
     if (Operands.size() > ErrorInfo + 1 &&
         ((ARM64Operand *)Operands[ErrorInfo + 1])->isTokenEqual("!"))
       MatchResult = Match_InvalidMemoryIndexedSImm9;
   // FALL THROUGH
   case Match_AddSubRegExtendSmall:
   case Match_AddSubRegExtendLarge:
+  case Match_AddSubSecondSource:
   case Match_InvalidMemoryIndexed8:
   case Match_InvalidMemoryIndexed16:
   case Match_InvalidMemoryIndexed32SImm7:
   case Match_InvalidMemoryIndexed64SImm7:
   case Match_InvalidMemoryIndexed128SImm7:
   case Match_InvalidImm0_7:
   case Match_InvalidImm0_15:
   case Match_InvalidImm0_31: