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

[Matthias Braun via llvm-commits]

MatzeB added a subscriber: MatzeB.
MatzeB added a comment.

The commit reads somewhat strange as it looks like a TODO file split into comments. I guess that is fine and too early to review until we have some real implementations. The cmake parts look good to me now. Nitpicks below.


================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:25
@@ +24,3 @@
+namespace llvm {
+// Forward declarations.
+class Constant;
----------------
Isn't this obvious?

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

================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:44
@@ +43,3 @@
+private:
+  // Interface use to lower the everything related to calls.
+  //  TargetLowering *CallLowering;
----------------
grammar.

================
Comment at: include/llvm/CodeGen/GlobalISel/IRTranslator.h:65
@@ +64,3 @@
+
+  /* a bunch ofmethods targeting ADD, SUB, etc.*/
+  // Return true if the translation was successful, false
----------------
grammar. Could use doxygen style // @{  .. // @} when describing a larger section of the file.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D15983