[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