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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Matthias,

> The commit reads somewhat strange as it looks like a TODO file split into comments.


This is really what this is :). I could it I guess to have just the cmake part, but we wouldn’t have anything to test the cmake then.

I didn’t quite get your comment on the "Makefile part”. I am admittedly not familiar with the internals of the build system. See my inlined comments.

Thanks,
-Quentin


================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:25
@@ +24,3 @@
+namespace llvm {
+// Forward declarations.
+class Constant;
----------------
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.

================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:31-38
@@ +30,10 @@
+
+// Technically the pass should run on an hypothetical MachineModule,
+// since it should translate Global into some sort of MachineGlobal.
+// The MachineGlobal should ultimately just be a transfer of ownership of
+// the interesting bits that are relevant to represent a global value.
+// That being said, we could investigate what would it cost to just duplicate
+// the information from the LLVM IR.
+// The idea is that ultimately we would be able to free up the memory used
+// by the LLVM IR as soon as the translation is over.
+class IRTranslator : public MachineFunctionPass {
----------------
MatzeB wrote:
> Before the class I would expect a doxygen (///) comment describing what the pass does.
> I think high-level concerns and discussions should rather go into the \file section of the .cpp file.
> 
> The same applies to most of the following comments, they are not doxygen and often describe a lot of high-level concerns instead of just describing what the function does...
Yeah, at this point, it is more todos than actual doxygen comments.

================
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
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D15983