[LLVMdev] [PATCH] use-diet for review

[heisenbug]

On Apr 29, 5:52 pm, Chris Lattner <clatt... at apple.com> wrote:
> 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  

No problem.

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

There are already two different contexts where tagging is needed.
(And my evil plans need another set of tags too ;-)
Moving this to its own header would be a piece of cake.
The question is, when? Today is my last chance to check
this stuff in?

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

I would like to avoid it, but there was a
reinterpret_cast<BasicBlock*>, which I have changed to
static_cast<BasicBlock*>.

/Users/ggreif/llvm-exp/use-diet/include/llvm/Instructions.h: In member
function 'llvm::BasicBlock* llvm::PHINode::getIncomingBlock(unsigned
int) const':
/Users/ggreif/llvm-exp/use-diet/include/llvm/Instructions.h:1448:
error: invalid static_cast from type 'llvm::Value*' to type
'llvm::BasicBlock*'

I regard the reinterpret_cast as a bigger evil than (#include +
static_cast). Also can you imagine that a file
includes Instructions.h but not BasicBlock.h?

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

Hmm, as I already answered to Dan, this is a bit historical, but at
the moment
setOperand would also do. In theory, though, Op<0>() and Op<1>() could
return references to different classes which could decay to different
subclasses of Value.

>
>     /// 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"?

Not really magic, there are two Value* in the argument list, which
corresponds to two Use objects to be allocated.

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

Done.

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

Done.

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

I can do that in a later step, no problem.

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

Fixed.


> +++ lib/Bitcode/Reader/BitcodeReader.cpp        (Arbeitskopie)
>
> +void BitcodeReaderValueList::resize(unsigned Desired) {
> +  if (Desired > Capacity)
> +  {
> Please put brace on same line as if.

Fixed.

>
> -          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?


It was never const-correct :-(. Just look at User.h in zour tree. I
will try
to restore this logical property though.

>
>   InvokeInst::~InvokeInst() {
> -  delete [] OperandList;
> +//  delete [] OperandList;
>   }
>
> Please remove commented out code.  Please move the trivial dtors to  
> the header file.

Removed completely. The compiler can synthesize a perfectly good one.

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

My compiler complained that specializing a (trait) template outside of
namespace llvm is not allowed. This kind of nesting allows template
specialization.

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

Already committed.

> -Chris


Thanks, Chris!

I shall be sending a fresh patch out soon.

Cheers,

    Gabor