[PATCH] getMangledTypeStr: clarify how it mangles types, and add tests

[Philip Reames]

Comments inline.  I like the general direction of the revised patch.


================
Comment at: lib/IR/Function.cpp:450
@@ -449,9 +449,3 @@
 /// mangling scheme used by 'any' types in intrinsic signatures.  The mangling
-/// of named types is simply their name.  Manglings for unnamed types consist
-/// of a prefix ('p' for pointers, 'a' for arrays, 'f_' for functions)
-/// combined with the mangling of their component types.  A vararg function
-/// type will have a suffix of 'vararg'.  Since function types can contain
-/// other function types, we close a function type mangling with suffix 'f'
-/// which can't be confused with it's prefix.  This ensures we don't have
-/// collisions between two unrelated function types. Otherwise, you might
-/// parse ffXX as f(fXX) or f(fX)X.  (X is a placeholder for any other type.)
+/// of named types is simply their name.  Manglings for unnamed types consist of
+/// a prefix ('p' for pointers, 'a' for arrays, 'f_' for functions) combined
----------------
Please separate line breaking differences into their own patch.  It's hard to tell what actually changed in this comment.

================
Comment at: lib/IR/Function.cpp:459
@@ -458,1 +458,3 @@
+/// floats, and vectors ('i', 'f', and 'v' prefix in most cases) fall back to
+/// the MVT codepath, where they could be mangled to 'x86mmx', for example.
 static std::string getMangledTypeStr(Type* Ty) {
----------------
x88mmx isn't what actually appears in your test?  Why the difference?  (Clarify in comment please.)


FYI - I expect you'll run into some problems actually lowering vectors gc.relocates.  It's not a case we've explored.  



================
Comment at: test/CodeGen/Generic/overloaded-intrinsic-name.ll:1
@@ +1,2 @@
+; RUN: llc < %s
+; Tests the name mangling performed by the codepath following
----------------
This is probably better as an opt -verify test.  Not sure CodeGen is the right place for it either.

Alternatively, you could go the other direction and adapt this to be a gc lowering specific test.  This would involve checking the assembly output, but might be reasonable tor exercising the vector path it seems like you're interested in.  

================
Comment at: test/CodeGen/Generic/overloaded-intrinsic-name.ll:4
@@ +3,3 @@
+; getMangledTypeStr(). Exercise the most general case,
+; llvm_anyptr_type, using gc.relocate and gc.statepoint.
+
----------------
You might want to clarify that the use of gc.* is arbitrary and that these are no GC tests.  It's mostly clear, but could be a bit more explicit.  

================
Comment at: test/CodeGen/Generic/overloaded-intrinsic-name.ll:7
@@ +6,3 @@
+; function and integer
+define i32* @test_iAny(i32* %v) {
+       %tok = call i32 (i1 ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()* @return_i1, i32 0, i32 0, i32 0, i32* %v)
----------------
Given that these don't have check statements, how do you judge correctness of the tests?

http://reviews.llvm.org/D6915

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/