[ARM64] Fix the misleading diagnostic on bad extend amount of reg+reg addressing mode.

[kevin.qin]

Summary

Hi Tim and Jim,

This patch is going to fix Bug 19502 (http://llvm.org/bugs/show_bug.cgi?id=19502). As comments described, I can't simply adding new diagnostic to asmoperand, so I implemented this in C code by a "hack" way. It's the best solution I can image without refactoring whole load/store implementation. Please review, thanks.

[t.p.northover]

Hi Kevin,

I don't think that degree of hackery is justified for a slightly better diagnostic. ARM64 already suffers from too much C++ code in the AsmParser, We want to work towards decreasing that rather than making it worse.

Cheers.

Tim.

[kevin.qin]

Hi Tim,

I agree with your point, that the AsmParser in ARM64 is already too
complexity. But for this bug, it can't avoid adding more codes in AsmParser
unless factoring whole address operand of load/store instructions. The
reason why I give up providing a suitably vague warning is that, there's
too many instructions sharing two diagnostics!

feeding diag.s in patch to llvm-mc built by trunk today, you will see

llvm-mc -triple=arm64 -mattr=+neon -assemble -show-encoding <diag.s

.text
<stdin>:1:13: error: index must be a multiple of 4 in range [0, 16380].
ldrb w1 , [x3, w3, sxtw #4]

^

<stdin>:2:13: error: index must be a multiple of 4 in range [0, 16380].
ldrh w1 , [x3, w3, sxtw #4]

^

<stdin>:3:13: error: index must be a multiple of 4 in range [0, 16380].
ldr w1 , [x3, w3, sxtw #4]

^

<stdin>:4:13: error: index must be a multiple of 8 in range [0, 32760].
ldr x1 , [x3, w3, sxtw #4]

^

<stdin>:5:13: error: index must be a multiple of 8 in range [0, 32760].
ldr b1 , [x3, w3, sxtw #4]

^

<stdin>:6:13: error: index must be a multiple of 8 in range [0, 32760].
ldr h1 , [x3, w3, sxtw #4]

^

<stdin>:7:13: error: index must be a multiple of 8 in range [0, 32760].
ldr s1 , [x3, w3, sxtw #4]

^

<stdin>:8:13: error: index must be a multiple of 8 in range [0, 32760].
ldr d1 , [x3, w3, sxtw #4]

^

<stdin>:9:13: error: index must be a multiple of 8 in range [0, 32760].
ldr q1 , [x3, w3, sxtw #1]

^

Each diagnostic will hold too many information and get end-users more
confused. Also, previous misleading diagnostic is for addressing mode [reg,
offset], the suggested offset ranges are incorrect
for 8/16/128 bit loading.
For example, when parsing "ldrb w1, [x3, #4096]", current diagnostic is "error:
index must be a multiple of 4 in range [0, 16380]." But correct index
should be any integer in [0, 4095].
This patch can fix this problem as well.

2014-05-08 19:29 GMT+08:00 Tim Northover <[e-mail]>:

Hi Kevin,

I don't think that degree of hackery is justified for a slightly better
diagnostic. ARM64 already suffers from too much C++ code in the AsmParser,
We want to work towards decreasing that rather than making it worse.

Cheers.

Tim.

http://reviews.llvm.org/D3662

[t.p.northover]

Hi Kevin,

Nevertheless, I'd prefer a vague diagnostic like "invalid offset in memory address" to putting this code in the AsmParser.

If you really want more accurate diagnostics then we need to do in some way that actually improves the code quality, not just the diagnostic quality.

Cheers.

Tim.

[kevin.qin]

Changed to the vague diagnostic as Tim suggested.

[t.p.northover]

Hi Kevin,

This patch looks fine. Thanks for changing it like that, I know you were keen on the better diagnostics.

Cheers.

Tim.

lib/Target/ARM64/ARM64InstrFormats.td

 
 class MemROAsmOperand<int sz> : AsmOperandClass {
   let Name = "MemoryRegisterOffset"#sz;
+  let DiagnosticType = "InvalidMemoryIndexed";
 }
 
 def MemROAsmOperand8 : MemROAsmOperand<8>;

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp

     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].");
+  case Match_InvalidMemoryIndexed:
+    return Error(Loc, "invalid offset in memory address.");
   case Match_InvalidMemoryIndexed8:
     return Error(Loc, "index must be an integer in range [0, 4095].");
   case Match_InvalidMemoryIndexed16:

     if (Operands.size() == ErrorInfo + 1 &&
         !((ARM64Operand *)Operands[ErrorInfo])->isImm() &&
         !Tok.startswith("stur") && !Tok.startswith("ldur")) {
-      // whether we want an Indexed64 or Indexed32 diagnostic depends on
-      // the register class of the previous operand. Default to 64 in case
-      // we see something unexpected.
-      MatchResult = Match_InvalidMemoryIndexed64;
-      if (ErrorInfo) {
-        ARM64Operand *PrevOp = (ARM64Operand *)Operands[ErrorInfo - 1];
-        if (PrevOp->isReg() &&
-            ARM64MCRegisterClasses[ARM64::GPR32RegClassID].contains(
-                PrevOp->getReg()))
-          MatchResult = Match_InvalidMemoryIndexed32;
-      }
+      // FIXME: Here we use a vague diagnostic for memory operand in many
+      // instructions of various formats. This diagnostic can be more accurate
+      // if splitting memory operand into many smaller operands to help
+      // diagnose.
+      MatchResult = Match_InvalidMemoryIndexed;
     }
     SMLoc ErrorLoc = ((ARM64Operand *)Operands[ErrorInfo])->getStartLoc();
     if (ErrorLoc == SMLoc())

test/MC/ARM64/diags.s

 
         ldur x0, [x1, #-257]
 
-; CHECK-ERRORS: error: index must be a multiple of 8 in range [0, 32760].
+; CHECK-ERRORS: error: invalid offset in memory address.
 ; CHECK-ERRORS:         ldr x0, [x0, #804]
 ; CHECK-ERRORS:                 ^
-; CHECK-ERRORS: error: index must be a multiple of 4 in range [0, 16380].
+; CHECK-ERRORS: error: invalid offset in memory address.
 ; CHECK-ERRORS:         ldr w0, [x0, #802]
 ; CHECK-ERRORS:                 ^
 ; CHECK-ERRORS: error: index must be an integer in range [-256, 255].

 ; CHECK-ERRORS:                   ^
 
 
+ldrb   w1, [x3, w3, sxtw #4]
+ldrh   w1, [x3, w3, sxtw #4]
+ldr    w1, [x3, w3, sxtw #4]
+ldr    x1, [x3, w3, sxtw #4]
+ldr    b1, [x3, w3, sxtw #4]
+ldr    h1, [x3, w3, sxtw #4]
+ldr    s1, [x3, w3, sxtw #4]
+ldr    d1, [x3, w3, sxtw #4]
+ldr    q1, [x3, w3, sxtw #1]
+
+; CHECK-ERRORS: error: invalid offset in memory address.
+; CHECK-ERRORS:ldrb   w1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: error: invalid offset in memory address.
+; CHECK-ERRORS:ldrh   w1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: error: invalid offset in memory address.
+; CHECK-ERRORS:ldr    w1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: error: invalid offset in memory address.
+; CHECK-ERRORS:ldr    x1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: error: invalid offset in memory address.
+; CHECK-ERRORS:ldr    b1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: invalid offset in memory address.
+; CHECK-ERRORS:ldr    h1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: invalid offset in memory address.
+; CHECK-ERRORS:ldr    s1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: invalid offset in memory address.
+; CHECK-ERRORS:ldr    d1, [x3, w3, sxtw #4]
+; CHECK-ERRORS:           ^
+; CHECK-ERRORS: invalid offset in memory address.
+; CHECK-ERRORS:ldr    q1, [x3, w3, sxtw #1]
+; CHECK-ERRORS:           ^
+
 ; Check that register offset addressing modes only accept 32-bit offset
 ; registers when using uxtw/sxtw extends. Everything else requires a 64-bit
 ; register.