[ARM64] Correct more bounds checks/diagnostics for arithmetic shift operands

[bsmith]

Summary

Patch to correct more bounds checks/diagnostics for arithmetic shift operands, this also make various invalid shifts actually invalid hence some test updates were needed.

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 question about this one:

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

2476–2478

What goes wrong if we just add the shift and let the isXYZ methods take care of it? It seems like it might be more consistent way to handle things in general.

[bsmith]

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

2476–2478

The shifts are encoded into a single integer in such a way that 0x3f is the maximum value for the shift, if you add one that exceeds this then it overflows into the shift type field in such a way that can turn it into a different valid shift type, if it doesn't assert first.

To be honest, I'm not sure why shifts/extracts/other things are encoded into a single integer like this, rather than using separate operand fields as AArch64 does, it seems to only cause headaches.

[t.p.northover]

The shifts are encoded into a single integer in such a way that 0x3f is the maximum value for the shift, if you add one that exceeds this then it overflows into the shift type field in such a way that can turn it into a different valid shift type, if it doesn't assert first.

OK, that makes sense for now. You should probably commit this one as
it is then, and we'll handle that combined operand separately.

Cheers.

Tim.

lib/Target/ARM64/ARM64InstrFormats.td

def MovImm64ShifterOperand : AsmOperandClass {
   let SuperClasses = [ShifterOperand];
   let Name = "MovImm64Shifter";
 }
 
 // Shifter operand for arithmetic register shifted encodings.
 class ArithmeticShifterOperand<int width> : AsmOperandClass {
   let SuperClasses = [ShifterOperand];
   let Name = "ArithmeticShifter" # width;
-  let PredicateMethod = "isArithmeticShifter";
+  let PredicateMethod = "isArithmeticShifter<" # width # ">";
   let RenderMethod = "addArithmeticShifterOperands";
   let DiagnosticType = "AddSubRegShift" # width;
 }
 
 def ArithmeticShifterOperand32 : ArithmeticShifterOperand<32>;
 def ArithmeticShifterOperand64 : ArithmeticShifterOperand<64>;
 
 // Shifter operand for logical vector 128/64-bit shifted encodings.

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

public:
   bool isMem() const override { return Kind == k_Memory; }
   bool isSysCR() const { return Kind == k_SysCR; }
   bool isPrefetch() const { return Kind == k_Prefetch; }
   bool isShifter() const { return Kind == k_Shifter; }
   bool isExtend() const {
     // lsl is an alias for UXTW but will be a parsed as a k_Shifter operand.
     if (isShifter()) {
       ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(Shifter.Val);
-      return ST == ARM64_AM::LSL;
+      return ST == ARM64_AM::LSL &&
+             ARM64_AM::getShiftValue(Shifter.Val) <= 4;
     }
     return Kind == k_Extend && ARM64_AM::getArithShiftValue(Shifter.Val) <= 4;
   }
   bool isExtend64() const {
     if (Kind != k_Extend)
       return false;
     // UXTX and SXTX require a 64-bit source register (the ExtendLSL64 class).
     ARM64_AM::ExtendType ET = ARM64_AM::getArithExtendType(Extend.Val);
     return ET != ARM64_AM::UXTX && ET != ARM64_AM::SXTX &&
            ARM64_AM::getArithShiftValue(Shifter.Val) <= 4;
   }
   bool isExtendLSL64() const {
     // lsl is an alias for UXTX but will be a parsed as a k_Shifter operand.
     if (isShifter()) {
       ARM64_AM::ShiftType ST = ARM64_AM::getShiftType(Shifter.Val);
-      return ST == ARM64_AM::LSL;
+      return ST == ARM64_AM::LSL &&
+             ARM64_AM::getShiftValue(Shifter.Val) <= 4;
     }
     if (Kind != k_Extend)
       return false;
     ARM64_AM::ExtendType ET = ARM64_AM::getArithExtendType(Extend.Val);
     return (ET == ARM64_AM::UXTX || ET == ARM64_AM::SXTX) &&
            ARM64_AM::getArithShiftValue(Shifter.Val) <= 4;
   }
 
+  template <unsigned width>
   bool isArithmeticShifter() const {
     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;
+    return (ST == ARM64_AM::LSL || ST == ARM64_AM::LSR ||
+           ST == ARM64_AM::ASR) && 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);
ARM64AsmParser::tryParseOptionalShift(OperandVector &Operands) {
 
   // 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("expected #imm after shift specifier");
     return MatchOperand_ParseFail;
   }
 
+  SMLoc E = SMLoc::getFromPointer(getLoc().getPointer() - 1);
+
+  // If we have an shift that is too large to encode then crudely pass it
+  // through as an invalid shift that is encodable so that we get consistant
+  // diagnostics rather than ones different from out of range 32-bit shifts.
   if ((MCE->getValue() & 0x3f) != MCE->getValue()) {
-    Error(ExprLoc, "immediate value too large for shifter operand");
-    return MatchOperand_ParseFail;
+    Operands.push_back(ARM64Operand::CreateShifter(ARM64_AM::InvalidShift, 0, S,
+                                                   E, getContext()));
+  } else {
+    Operands.push_back(ARM64Operand::CreateShifter(ShOp, MCE->getValue(), S,
+                                                   E, getContext()));
   }
 
-  SMLoc E = SMLoc::getFromPointer(getLoc().getPointer() - 1);
-  Operands.push_back(
-      ARM64Operand::CreateShifter(ShOp, MCE->getValue(), S, E, getContext()));
   return MatchOperand_Success;
 }
 
 /// tryParseOptionalExtend - Some operands take an optional extend argument. Parse
 /// them if present.
 ARM64AsmParser::OperandMatchResultTy
 ARM64AsmParser::tryParseOptionalExtend(OperandVector &Operands) {
   const AsmToken &Tok = Parser.getTok();

test/MC/ARM64/arithmetic-encoding.s

 ;==---------------------------------------------------------------------------==
 ; Add/Subtract register with (optional) shift
 ;==---------------------------------------------------------------------------==
 
   add w12, w13, w14
   add x12, x13, x14
   add w12, w13, w14, lsl #12
   add x12, x13, x14, lsl #12
-  add w12, w13, w14, lsr #42
   add x12, x13, x14, lsr #42
-  add w12, w13, w14, asr #39
   add x12, x13, x14, asr #39
 
 ; CHECK: add w12, w13, w14           ; encoding: [0xac,0x01,0x0e,0x0b]
 ; CHECK: add x12, x13, x14           ; encoding: [0xac,0x01,0x0e,0x8b]
 ; CHECK: add w12, w13, w14, lsl #12  ; encoding: [0xac,0x31,0x0e,0x0b]
 ; CHECK: add x12, x13, x14, lsl #12  ; encoding: [0xac,0x31,0x0e,0x8b]
-; CHECK: add w12, w13, w14, lsr #42  ; encoding: [0xac,0xa9,0x4e,0x0b]
 ; CHECK: add x12, x13, x14, lsr #42  ; encoding: [0xac,0xa9,0x4e,0x8b]
-; CHECK: add w12, w13, w14, asr #39  ; encoding: [0xac,0x9d,0x8e,0x0b]
 ; CHECK: add x12, x13, x14, asr #39  ; encoding: [0xac,0x9d,0x8e,0x8b]
 
   sub w12, w13, w14
   sub x12, x13, x14
   sub w12, w13, w14, lsl #12
   sub x12, x13, x14, lsl #12
-  sub w12, w13, w14, lsr #42
   sub x12, x13, x14, lsr #42
-  sub w12, w13, w14, asr #39
   sub x12, x13, x14, asr #39
 
 ; CHECK: sub w12, w13, w14           ; encoding: [0xac,0x01,0x0e,0x4b]
 ; CHECK: sub x12, x13, x14           ; encoding: [0xac,0x01,0x0e,0xcb]
 ; CHECK: sub w12, w13, w14, lsl #12  ; encoding: [0xac,0x31,0x0e,0x4b]
 ; CHECK: sub x12, x13, x14, lsl #12  ; encoding: [0xac,0x31,0x0e,0xcb]
-; CHECK: sub w12, w13, w14, lsr #42  ; encoding: [0xac,0xa9,0x4e,0x4b]
 ; CHECK: sub x12, x13, x14, lsr #42  ; encoding: [0xac,0xa9,0x4e,0xcb]
-; CHECK: sub w12, w13, w14, asr #39  ; encoding: [0xac,0x9d,0x8e,0x4b]
 ; CHECK: sub x12, x13, x14, asr #39  ; encoding: [0xac,0x9d,0x8e,0xcb]
 
   adds w12, w13, w14
   adds x12, x13, x14
   adds w12, w13, w14, lsl #12
   adds x12, x13, x14, lsl #12
-  adds w12, w13, w14, lsr #42
   adds x12, x13, x14, lsr #42
-  adds w12, w13, w14, asr #39
   adds x12, x13, x14, asr #39
 
 ; CHECK: adds w12, w13, w14          ; encoding: [0xac,0x01,0x0e,0x2b]
 ; CHECK: adds x12, x13, x14          ; encoding: [0xac,0x01,0x0e,0xab]
 ; CHECK: adds w12, w13, w14, lsl #12 ; encoding: [0xac,0x31,0x0e,0x2b]
 ; CHECK: adds x12, x13, x14, lsl #12 ; encoding: [0xac,0x31,0x0e,0xab]
-; CHECK: adds w12, w13, w14, lsr #42 ; encoding: [0xac,0xa9,0x4e,0x2b]
 ; CHECK: adds x12, x13, x14, lsr #42 ; encoding: [0xac,0xa9,0x4e,0xab]
-; CHECK: adds w12, w13, w14, asr #39 ; encoding: [0xac,0x9d,0x8e,0x2b]
 ; CHECK: adds x12, x13, x14, asr #39 ; encoding: [0xac,0x9d,0x8e,0xab]
 
   subs w12, w13, w14
   subs x12, x13, x14
   subs w12, w13, w14, lsl #12
   subs x12, x13, x14, lsl #12
-  subs w12, w13, w14, lsr #42
   subs x12, x13, x14, lsr #42
-  subs w12, w13, w14, asr #39
   subs x12, x13, x14, asr #39
 
 ; CHECK: subs w12, w13, w14          ; encoding: [0xac,0x01,0x0e,0x6b]
 ; CHECK: subs x12, x13, x14          ; encoding: [0xac,0x01,0x0e,0xeb]
 ; CHECK: subs w12, w13, w14, lsl #12 ; encoding: [0xac,0x31,0x0e,0x6b]
 ; CHECK: subs x12, x13, x14, lsl #12 ; encoding: [0xac,0x31,0x0e,0xeb]
-; CHECK: subs w12, w13, w14, lsr #42 ; encoding: [0xac,0xa9,0x4e,0x6b]
 ; CHECK: subs x12, x13, x14, lsr #42 ; encoding: [0xac,0xa9,0x4e,0xeb]
-; CHECK: subs w12, w13, w14, asr #39 ; encoding: [0xac,0x9d,0x8e,0x6b]
 ; CHECK: subs x12, x13, x14, asr #39 ; encoding: [0xac,0x9d,0x8e,0xeb]
 
 ; Check use of upper case register names rdar://14354073
   add X2, X2, X2
 ; CHECK: add x2, x2, x2              ; encoding: [0x42,0x00,0x02,0x8b]
 
 ;==---------------------------------------------------------------------------==
 ; Add/Subtract with (optional) extend

test/MC/ARM64/diags.s

 ; CHECK-ERRORS: error: unpredictable STR instruction, writeback base is also a source
 ; CHECK-ERRORS:   str w2, [x2, #8]!
 ; CHECK-ERRORS:       ^
 
 ; The validity checking for shifted-immediate operands.  rdar://13174476
 ; Where the immediate is out of range.
   add w1, w2, w3, lsr #75
 
-; CHECK-ERRORS: error: immediate value too large for shifter operand
+; 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:        orr w0, w0, w0, lsl #32
 ; CHECK-ERRORS:                        ^