[LLVMdev] [PATCH] use-diet for review

[Chris Lattner]

On Apr 29, 2008, at 1:27 AM, Gabor Greif wrote:

> Hi all,
>
> I have reported more than enough about the space savings achieved
> and the associated costs, here comes the current patch for review.
>
> Since this one is substantially smaller than the previous one, I did
> not cut it in pieces. The front part is about headers and the rest
> the .cpp and other files.

Hi Gabor,

Thanks for working on this!  A 15% memory savings is huge.  As I  
mentioned, a pivotal file is missing from your diff.  Here are some  
thoughts and questions to start with:

  // 
= 
= 
=---------------------------------------------------------------------- 
===//
+//                          Generic Tagging Functions
+// 
= 
= 
=---------------------------------------------------------------------- 
===//
+
+/// Tag - generic tag type for (at least 32 bit) pointers
+enum Tag { noTag, tagOne, tagTwo, tagThree };
Why such a generic pointer tagging mechanism?  This should either be  
specific to the application or in a shared location (e.g. include/llvm/ 
Support).

+++ include/llvm/Instructions.h	(Arbeitskopie)
@@ -21,10 +21,10 @@
  #include "llvm/DerivedTypes.h"
  #include "llvm/ParameterAttributes.h"
+#include "llvm/BasicBlock.h"

Why the #include?  Please avoid this if possible.

Why are operand lists stored as templates instead of arrays? "Op<0>"  
is very strange to me.

    /// Constructors - These two constructors are convenience methods  
because one
    /// and two index getelementptr instructions are so common.
    static GetElementPtrInst *Create(Value *Ptr, Value *Idx,
                                     const std::string &Name = "",  
Instruction *InsertBefore = 0) {
-    return new(2/*FIXME*/) GetElementPtrInst(Ptr, Idx, Name,  
InsertBefore);
+    return new(2) GetElementPtrInst(Ptr, Idx, Name, InsertBefore);
    }

What is the magic "2"?

+GetElementPtrInst::GetElementPtrInst(const GetElementPtrInst &GEPI)
+  : Instruction(reinterpret_cast<const Type*>(GEPI.getType()),  
GetElementPtr,
+                OperandTraits<GetElementPtrInst>::op_end(this) -  
GEPI.getNumOperands(),
+                GEPI.getNumOperands()) {
+  Use *OL = OperandList;
+  Use *GEPIOL = GEPI.OperandList;
+  for (unsigned i = 0, E = NumOperands; i != E; ++i)
+    OL[i].init(GEPIOL[i], this);
+}
Please just move methods like this out of line when possible.

+DEFINE_TRANSPARENT_OPERAND_ACCESSORS(CallInst, Value)
+//void CallInst::operator delete(void *it) {
+//  OperandTraits<CallInst>::op_begin(static_cast<CallInst*>(it));
+//}
+
Please remove the commented out code.
+++ include/llvm/User.h	(Arbeitskopie)
@@ -23,15 +23,203 @@
+/ 
*= 
= 
= 
= 
= 
= 
========================================================================
+   -----------------------------------------------------------------
+   --- Interaction and relationship between User and Use objects ---
+   -----------------------------------------------------------------
Documentation like this is awesome, but should really go in the  
programmer's manual (e.g. see the discussion on type resolution), not  
in the header.

+++ lib/Bitcode/Reader/BitcodeReader.h	(Arbeitskopie)
+    if (Idx >= size()) {
+      // Insert a bunch of null values.
+        resize(Idx * 2 + 1);
+    }
strange indentation
+++ lib/Bitcode/Reader/BitcodeReader.cpp	(Arbeitskopie)

+void BitcodeReaderValueList::resize(unsigned Desired) {
+  if (Desired > Capacity)
+  {
Please put brace on same line as if.

-          Constant *C= ConstantExpr::getFCmp(FCmpInst::FCMP_OEQ,
-              const_cast<Constant*>(CP1->getOperand(i)),
-              const_cast<Constant*>(CP2->getOperand(i)));
+          Constant *C = ConstantExpr::getFCmp(FCmpInst::FCMP_OEQ,
+                                              CP1->getOperand(i),
+                                              CP2->getOperand(i));
getOperand() is no longer const-correct?

  InvokeInst::~InvokeInst() {
-  delete [] OperandList;
+//  delete [] OperandList;
  }

Please remove commented out code.  Please move the trivial dtors to  
the header file.


+++ lib/VMCore/Constants.cpp	(Arbeitskopie)
+namespace llvm {
  // We declare several classes private to this file, so use an  
anonymous
  // namespace
  namespace {
Why wrap the anon namespace in namespace llvm?  It is still anonymous.


+++ lib/Support/MemoryBuffer.cpp	(Arbeitskopie)
This part is unrelated, if correct, plz commit independently.
-Chris