[PATCH] SymbolRewriter: introduce the SymbolRewriter pass

[Saleem Abdulrasool]

Given that the move will simplify the diff, I've addressed the inline comments and move into Utils from its own directory.  I'll see about restructuring the descriptors to private from public.  If clang ever needs to generate the descriptors, we can make them public then.  Better to make as many of the types opaque as possible.

================
Comment at: include/llvm/CodeGen/Passes.h:600
@@ -596,1 +599,3 @@
+  ModulePass *createRewriteSymbolsPass();
+  ModulePass *createRewriteSymbolsPass(SymbolRewriter::RewriteDescriptorList &);
 } // End llvm namespace
----------------
grosbach wrote:
> Nothing calls the second version.
Correct.  Nothing calls it yet, the idea was to add this in this form to permit clang to read the file and provide the descriptor list to LLVM so that we do not have to read the file in the backend.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:1
@@ +1,2 @@
+
+#ifndef LLVM_TRANSFORMS_UTILS_SYMBOL_REWRITER_H
----------------
grosbach wrote:
> Needs file level comment header. Should include a fairly substantive description of what the pass does.
Added something to cover this.  Happy to enhance it as necessary.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:18
@@ +17,3 @@
+
+namespace SymbolRewriter {
+class RewriteDescriptor : public ilist_node<RewriteDescriptor> {
----------------
grosbach wrote:
> Why is there a SymbolRewriter namespace? The classes themselves should already provide the necessary scoping.
Well, this is being push up towards clang, but I don't see why it would need to be able to see the ModulePass subclass.  I thought that the namespace was a good solution, but Im not tied to it.  How would you prefer to handle this situation?

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:26
@@ +25,3 @@
+public:
+  enum RewriteDescriptorType {
+    RDT_Invalid,
----------------
grosbach wrote:
> Comments on what these meen would be very helpful.
> 
> Why the RDT_ prefix? They're already namespaced inside the class.
This was written way before we adopted C++11.  I can use an enum class now.  Comments added.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:52
@@ +51,3 @@
+  ExplicitRewriteFunctionDescriptor(const StringRef S, const StringRef T,
+                                    const bool Naked)
+    : RewriteDescriptor(RDT_Function),
----------------
grosbach wrote:
> const on a non-reference argument?
StringRefs shouldn't be const nor refs.  Fixed.

================
Comment at: include/llvm/Transforms/Utils/SymbolRewriter.h:63
@@ +62,3 @@
+
+class PatternRewriteFunctionDescriptor : public RewriteDescriptor {
+public:
----------------
grosbach wrote:
> In general, these classes all need some comments explaining how they fit together.
Sure.  Added a block comment describing them all.  Let me know if you would like to enhance that in any way.

================
Comment at: lib/Transforms/Makefile:12
@@ -12,1 +11,3 @@
+PARALLEL_DIRS = Utils Instrumentation Scalar InstCombine IPO Vectorize Hello \
+		ObjCARC SymbolRewriter
 
----------------
grosbach wrote:
> Why is this in its own subdirectory? The header is in Utils (which seems to make sense), so the implementation should be, too, I'd think.
Thought it was something that was less of a general purpose utility for the other transformations.  Happily moved as it greatly simplifies the build aspect.

================
Comment at: lib/Transforms/SymbolRewriter/SymbolRewriter.cpp:474
@@ +473,3 @@
+
+  if (!(Transform.empty() ^ Target.empty())) {
+    YS.printError(Descriptor, "exactly one of transform or target must be specified");
----------------
grosbach wrote:
> Eh? Bitwise operator on the return values of empty()? That's a non-standard idiom. Rewriting in terms of boolean operators would make more sense.
Sure.  Done.

================
Comment at: lib/Transforms/SymbolRewriter/SymbolRewriter.cpp:495
@@ +494,3 @@
+  RewriteSymbols();
+  RewriteSymbols(llvm::SymbolRewriter::RewriteDescriptorList &DL);
+
----------------
grosbach wrote:
> We have a "using namespace llvm" at the top. No need for the explicit scope operator here.
Fixed.

================
Comment at: lib/Transforms/SymbolRewriter/SymbolRewriter.cpp:544
@@ +543,3 @@
+llvm::ModulePass *
+llvm::createRewriteSymbolsPass(SymbolRewriter::RewriteDescriptorList &DL) {
+  return new RewriteSymbols(DL);
----------------
grosbach wrote:
> Nothing calls this version of the factory function?
This is meant for the integration with clang.  I figured that adding that up front would be better.  Im happy to remove it for the time being.

http://reviews.llvm.org/D3161