[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