[PATCH] D15983: [GlobalISel] Add the proper cmake plumbing.

[Matthias Braun via llvm-commits]

MatzeB added inline comments.

================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:25
@@ +24,3 @@
+namespace llvm {
+// Forward declarations.
+class Constant;
----------------
qcolombet wrote:
> MatzeB wrote:
> > Isn't this obvious?
> I don’t know how common it is to use that kind of comments, but I personally like to call it out so that it is obvious that this section is for forward declarations and nothing else.
> I can take it out if you prefer.
It's probably a personal thing but I often think a reduced amount of  information is better than large amounts of obvious/unnecessary information as there is always a (really really tiny in this case) cost of reading/processing it. Sure this particular example is probably not even worth the time thinking or discussing it in a review. So no objection from me one way or the other.

================
Comment at: lib/CodeGen/GlobalISel/LLVMBuild.txt:18-22
@@ +17,6 @@
+
+[component_0]
+type = Library
+name = GlobalISel
+parent = CodeGen
+required_libraries = Analysis CodeGen Core MC Support Target TransformUtils
----------------
qcolombet wrote:
> MatzeB wrote:
> > Does this give us a way to disable/enable GlobalISel in the Makefiles? If not maybe just leave out of the Makefile system for now as there is a good chance that the Makefiles are removed before GlobalISel is relevant for most people.
> What do you mean “in the Makefiles”?
> 
> If you do add the dependency on GlobalISel, the libLLVMGlobalISel should be built, right?
> 
> Moreover, I thought those LLVMBuild.txt files were mandatory.
Hmm I assumed the LLVMBuild.txt is only used for the Makefile build, because when I added some simple libraries/executables in my personal experiments everything worked fine without them and the information looked like a duplication of everything in CMakeLists.txt. But you are right I just checked again and the cmake build is doing something with the information as well, so they are apparently necessary for cmake and Makefile builds.


Repository:
  rL LLVM

http://reviews.llvm.org/D15983