[PATCH] D20727: [mips] Weaken asm predicate for memory offsets

[Daniel Sanders via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with one nit about a comment.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1077
@@ -1076,2 +1076,3 @@
   template <unsigned Bits, unsigned ShiftAmount = 0>
+  // Allow relocation operators.
   bool isMemWithSimmOffset() const {
----------------
I don't think we need this comment but if we do want to keep it then please put it in front of the 'template'

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1080
@@ +1079,3 @@
+    return isMem() && getMemBase()->isGPRAsmReg() &&
+           (isa<MCTargetExpr>(getMemOff()) ||
+            (isConstantMemOff() &&
----------------
Hmm... This isn't quite correct but it's no less correct than the other predicates. I'm happy for this to be committed as-is but we need to fix this and a couple other predicates afterwards.

My concern is that checks for specific expression nodes will be confused by any variety. For example isConstantMemOff() is false for '1+1' because the top-level node is a MCBinaryExpr instead of an MCConstantExpr. Similarly, isMemWithSimmOffset() will unnecessarily reject things like '%lo(foo) + 2' when 'foo' is known. When 'foo' is unknown it will correctly reject it because %lo(foo)+2 is not a relocatable expression (it should be %lo(foo+2)).

I think we should do this kind of test with something along the lines of:
  int64_t Offset;
  if (getMemOff()->evaluateAsAbsolute(Offset) && isShiftedInt<Bits, ShiftAmount>(Offset))
    return true;
  return getMemOff()->evaluateAsRelocatable(Value, ...);
Possibly with some checks that MCValue::RefKind agrees with the size of the field in the instruction


Repository:
  rL LLVM

http://reviews.llvm.org/D20727