[ARM64] Add proper bounds checking/diagnostics to logical shifts

[bsmith]

Summary

Patch to add proper bounds checking/diagnostics to logical shifts, this also means we can get rid of yet more AsmParser hacks that do this manually.

As with my previous patches, there is no testcase for this as it is part of the merge of MC/AArch64/basic-a64-diagnostics.s.

[t.p.northover]

Hi Bradley,

I've got one nit-pick here. Not a correctness issue or anything...

lib/Target/ARM64/ARM64InstrFormats.td

553–558

I think it'd be better to pass the entire logical_shiftXY operand through here.

I view casting strings to particular Record types as a last ditch solution when you'd otherwise have to pass dozens of them up the hierarchy.

[bsmith]

Fix to pass through operand types directly, as suggested.

[t.p.northover]

Hi Bradley,

lib/Target/ARM64/ARM64InstrFormats.td

class ArithmeticShifterOperand<int width> : AsmOperandClass {
   let PredicateMethod = "isArithmeticShifter<" # width # ">";
   let RenderMethod = "addArithmeticShifterOperands";
   let DiagnosticType = "AddSubRegShift" # width;
 }
 
 def ArithmeticShifterOperand32 : ArithmeticShifterOperand<32>;
 def ArithmeticShifterOperand64 : ArithmeticShifterOperand<64>;
 
+// Shifter operand for logical register shifted encodings.
+class LogicalShifterOperand<int width> : AsmOperandClass {
+  let SuperClasses = [ShifterOperand];
+  let Name = "LogicalShifter" # width;
+  let PredicateMethod = "isLogicalShifter<" # width # ">";
+  let RenderMethod = "addLogicalShifterOperands";
+  let DiagnosticType = "AddSubRegShift" # width;
+}
+
+def LogicalShifterOperand32 : LogicalShifterOperand<32>;
+def LogicalShifterOperand64 : LogicalShifterOperand<64>;
+
 // 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";

 }
 
 def arith_shifted_reg32 : arith_shifted_reg<i32, GPR32, 32>;
 def arith_shifted_reg64 : arith_shifted_reg<i64, GPR64, 64>;
 
 // An arithmetic shifter operand:
 //  {7-6} - shift type: 00 = lsl, 01 = lsr, 10 = asr, 11 = ror
 //  {5-0} - imm6
-def logical_shift : Operand<i32> {
+class logical_shift<int width> : Operand<i32> {
   let PrintMethod = "printShifter";
-  let ParserMatchClass = ShifterOperand;
+  let ParserMatchClass = !cast<AsmOperandClass>(
+                         "LogicalShifterOperand" # width);
 }
 
-class logical_shifted_reg<ValueType Ty, RegisterClass regclass>
+def logical_shift32 : logical_shift<32>;
+def logical_shift64 : logical_shift<64>;
+
+class logical_shifted_reg<ValueType Ty, RegisterClass regclass, Operand shiftop>
     : Operand<Ty>,
       ComplexPattern<Ty, 2, "SelectLogicalShiftedRegister", []> {
   let PrintMethod = "printShiftedRegister";
-  let MIOperandInfo = (ops regclass, logical_shift);
+  let MIOperandInfo = (ops regclass, shiftop);
 }
 
-def logical_shifted_reg32 : logical_shifted_reg<i32, GPR32>;
-def logical_shifted_reg64 : logical_shifted_reg<i64, GPR64>;
+def logical_shifted_reg32 : logical_shifted_reg<i32, GPR32, logical_shift32>;
+def logical_shifted_reg64 : logical_shifted_reg<i64, GPR64, logical_shift64>;
 
 // A logical vector shifter operand:
 //  {7-6} - shift type: 00 = lsl
 //  {5-0} - imm6: #0, #8, #16, or #24
 def logical_vec_shift : Operand<i32> {
   let PrintMethod = "printShifter";
   let EncoderMethod = "getVecShifterOpValue";
   let ParserMatchClass = LogicalVecShifterOperand;
 }

lib/Target/ARM64/ARM64InstrInfo.td

def : InstAlias<"tst $src1, $src2",
                 (ANDSXri XZR, GPR64:$src1, logical_imm64:$src2)>;
 
 def : InstAlias<"tst $src1, $src2",
                 (ANDSWrs WZR, GPR32:$src1, GPR32:$src2, 0)>;
 def : InstAlias<"tst $src1, $src2",
                 (ANDSXrs XZR, GPR64:$src1, GPR64:$src2, 0)>;
 
 def : InstAlias<"tst $src1, $src2, $sh",
-                (ANDSWrs WZR, GPR32:$src1, GPR32:$src2, logical_shift:$sh)>;
+                (ANDSWrs WZR, GPR32:$src1, GPR32:$src2, logical_shift32:$sh)>;
 def : InstAlias<"tst $src1, $src2, $sh",
-                (ANDSXrs XZR, GPR64:$src1, GPR64:$src2, logical_shift:$sh)>;
+                (ANDSXrs XZR, GPR64:$src1, GPR64:$src2, logical_shift64:$sh)>;
 
 def : InstAlias<"mvn $Wd, $Wm",
                 (ORNWrs GPR32:$Wd, WZR, GPR32:$Wm, 0)>;
 def : InstAlias<"mvn $Xd, $Xm",
                 (ORNXrs GPR64:$Xd, XZR, GPR64:$Xm, 0)>;
 
 def : InstAlias<"mvn $Wd, $Wm, $sh",
-                (ORNWrs GPR32:$Wd, WZR, GPR32:$Wm, logical_shift:$sh)>;
+                (ORNWrs GPR32:$Wd, WZR, GPR32:$Wm, logical_shift32:$sh)>;
 def : InstAlias<"mvn $Xd, $Xm, $sh",
-                (ORNXrs GPR64:$Xd, XZR, GPR64:$Xm, logical_shift:$sh)>;
+                (ORNXrs GPR64:$Xd, XZR, GPR64:$Xm, logical_shift64:$sh)>;
 
 def : Pat<(not GPR32:$Wm), (ORNWrr WZR, GPR32:$Wm)>;
 def : Pat<(not GPR64:$Xm), (ORNXrr XZR, GPR64:$Xm)>;
 
 
 //===----------------------------------------------------------------------===//
 // One operand data processing instructions.
 //===----------------------------------------------------------------------===//

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

if (!isShifter())
       return false;
 
     // An arithmetic shifter is LSL, LSR, or ASR.
     ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(Shifter.Val);
     return (ST == ARM64_AM::LSL || ST == ARM64_AM::LSR ||
            ST == ARM64_AM::ASR) && ARM64_AM::getShiftValue(Shifter.Val) < width;
   }
 
+  template <unsigned width>
+  bool isLogicalShifter() const {
+    if (!isShifter())
+      return false;
+
+    // A logical shifter is LSL, LSR, ASR or ROR.
+    ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(Shifter.Val);
+    return (ST == ARM64_AM::LSL || ST == ARM64_AM::LSR ||
+           ST == ARM64_AM::ASR || ST == ARM64_AM::ROR) &&
+           ARM64_AM::getShiftValue(Shifter.Val) < width;
+  }
+
   bool isMovImm32Shifter() const {
     if (!isShifter())
       return false;
 
     // A MOVi shifter is LSL of 0, 16, 32, or 48.
     ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(Shifter.Val);
     if (ST != ARM64_AM::LSL)
       return false;
void addShifterOperands(MCInst &Inst, unsigned N) const {
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
   }
 
   void addArithmeticShifterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
   }
 
+  void addLogicalShifterOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    Inst.addOperand(MCOperand::CreateImm(getShifter()));
+  }
+
   void addMovImm32ShifterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
   }
 
   void addMovImm64ShifterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::CreateImm(getShifter()));
case ARM64::STRXpre: {
     break;
   }
   }
 
   // Now check immediate ranges. Separate from the above as there is overlap
   // in the instructions being checked and this keeps the nested conditionals
   // to a minimum.
   switch (Inst.getOpcode()) {
-  case ARM64::ANDWrs:
-  case ARM64::ANDSWrs:
-  case ARM64::EORWrs:
-  case ARM64::ORRWrs: {
-    if (!Inst.getOperand(3).isImm())
-      return Error(Loc[3], "immediate value expected");
-    int64_t shifter = Inst.getOperand(3).getImm();
-    ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(shifter);
-    if (ST == ARM64_AM::LSL && shifter > 31)
-      return Error(Loc[3], "shift value out of range");
-    return false;
-  }
   case ARM64::ADDSWri:
   case ARM64::ADDSXri:
   case ARM64::ADDWri:
   case ARM64::ADDXri:
   case ARM64::SUBSWri:
   case ARM64::SUBSXri:
   case ARM64::SUBWri:
   case ARM64::SUBXri: {

test/MC/ARM64/diags.s

; Where the immediate is out of range.
   add w1, w2, w3, lsr #75
 
 ; CHECK-ERRORS: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
 ; CHECK-ERRORS: add w1, w2, w3, lsr #75
 ; CHECK-ERRORS:                      ^
 
 ; logical instructions on 32-bit regs with shift > 31 is not legal
 orr w0, w0, w0, lsl #32
-; CHECK-ERRORS: error: shift value out of range
+; CHECK-ERRORS: error: expected 'lsl', 'lsr' or 'asr' with optional integer in range [0, 31]
 ; CHECK-ERRORS:        orr w0, w0, w0, lsl #32
 ; CHECK-ERRORS:                        ^
 eor w0, w0, w0, lsl #32
-; CHECK-ERRORS: error: shift value out of range
+; CHECK-ERRORS: error: expected 'lsl', 'lsr' or 'asr' with optional integer in range [0, 31]
 ; CHECK-ERRORS:        eor w0, w0, w0, lsl #32
 ; CHECK-ERRORS:                        ^
 and w0, w0, w0, lsl #32
-; CHECK-ERRORS: error: shift value out of range
+; CHECK-ERRORS: error: expected 'lsl', 'lsr' or 'asr' with optional integer in range [0, 31]
 ; CHECK-ERRORS:        and w0, w0, w0, lsl #32
 ; CHECK-ERRORS:                        ^
 ands w0, w0, w0, lsl #32
-; CHECK-ERRORS: error: shift value out of range
+; CHECK-ERRORS: error: expected 'lsl', 'lsr' or 'asr' with optional integer in range [0, 31]
 ; CHECK-ERRORS:        ands w0, w0, w0, lsl #32
 ; CHECK-ERRORS:                        ^
 
 ; Relocated expressions should not be accepted for 32-bit adds or sub (imm)
 add w3, w5, sym@PAGEOFF
 ; CHECK-ERRORS: error: invalid immediate expression
 ; CHECK-ERRORS: add w3, w5, sym@PAGEOFF
 ; CHECK-ERRORS:             ^