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

[David Peixotto]

Thanks for the patch. Overall I like the direction you are going. Please see specific comments below.

================
Comment at: include/llvm/MC/ConstantPools.h:29
@@ -29,1 +28,3 @@
+  unsigned MaxAlignment;
+  typedef SmallVector<std::tuple<MCSymbol *, const MCExpr *, unsigned>, 4> EntryVecTy;
   EntryVecTy Entries;
----------------
This type is getting a little hard to read. I suggest creating a small struct to represent the constant pool entries and name each field with something meaningful (e.g. Expr, Size, etc). 

================
Comment at: include/llvm/MC/ConstantPools.h:41
@@ -40,1 +40,3 @@
+  const MCExpr *addEntry(const MCExpr *Value, MCContext &Context,
+                         unsigned Size = 4);
 
----------------
I think we should not use default parameters here so that it is clear that in the API that size is important.

================
Comment at: include/llvm/MC/ConstantPools.h:75
@@ -73,1 +74,3 @@
+  const MCExpr *addEntry(MCStreamer &Streamer, const MCExpr *Expr,
+                         unsigned Size = 4);
 
----------------
Ditto for this default.  We can modify the ARMTargetStreamer to always pass 4 to this call since it only ever emits 4-byte constants.

================
Comment at: lib/MC/ConstantPools.cpp:34
@@ +33,3 @@
+    unsigned Size = std::get<2>(*I);
+    if (Alignment && Size > Alignment)
+      Streamer.EmitCodeAlignment(Size);
----------------
I think you can simplify the code here by just always emitting the alignment before each entry (EmitCodeAlignment(Size)). You may end up with some redundant alignment directives in the .s file, but it should not cause any code bloat in the final binary because the assembler already keeps track of the alignment as it goes.

This will simplify the logic and remove the need to keep track of MaxAlignment.

================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:3114
@@ -3111,1 +3113,3 @@
       }
+      int64_t Simm = Imm << ShiftAmt;
+      if (!IsXReg && (Simm >= 1LL << 32 || Simm < INT32_MIN))
----------------
Why int64_t here instead of uint64_t? It looks like the code above uses unsigned. Consider using the APInt to make it clear what condition you are checking (e.g. something like APint(Simm, 64).countActiveBits() >= 33)

http://reviews.llvm.org/D4279