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

Bradley Smith bradley.smith at arm.com
Thu May 8 05:11:44 PDT 2014


================
Comment at: lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp:581-583
@@ +580,5 @@
+      unsigned Shift = ShiftedImm.ShiftAmount;
+      if (ARM64_AM::getShiftType(Shift) != ARM64_AM::LSL ||
+         (ARM64_AM::getShiftValue(Shift) != 0 &&
+          ARM64_AM::getShiftValue(Shift) != 12))
+        return false;
----------------
Tim Northover wrote:
> 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).
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..

================
Comment at: lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp:2306-2307
@@ +2305,4 @@
+/// tryParseAddSubImm - Parse ADD/SUB shifted immediate operand
+ARM64AsmParser::OperandMatchResultTy
+ARM64AsmParser::tryParseAddSubImm(OperandVector &Operands) {
+  SMLoc S = getLoc();
----------------
Tim Northover wrote:
> 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?
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.

http://reviews.llvm.org/D3665






More information about the llvm-commits mailing list