[PATCH] MC: support different sized constants in constant pools

David Peixotto dpeixott at codeaurora.org
Thu Jul 17 10:50:57 PDT 2014


Thanks, this is looking much better. I had one minor suggestion and a question about the APInt usage.

Also, for future patches it would be best to include more context in the patch as described here: http://llvm.org/docs/Phabricator.html. It makes it easier to review.

Thanks for all your work.

================
Comment at: include/llvm/MC/ConstantPools.h:44
@@ -36,3 +43,3 @@
   // \param Value is the new entry to put in the constant pool.
   //
   // \returns a MCExpr that references the newly inserted value
----------------
Add comment for Size param (size in bytes)

================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3120
@@ +3119,3 @@
+      // check whether the immediate is a (un)signed 32-bit value
+      if (!IsXReg && Simm.getNumSignBits() - Simm.isNegative() < 32)
+        return Error(Loc, "Immediate too large for register");
----------------
This test still looks a bit hard to understand for me. Are you just checking if the value fits into 32 bits? If so I think APInt::isIntN(32) would be more readable.

http://reviews.llvm.org/D4279






More information about the llvm-commits mailing list