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. Cheers, Gabor -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: use-diet-patch-2904.diff URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080429/7b1edcfd/attachment.ksh>
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, I don't see OperandTraits.h. -Chris
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
Hi Gabor,
Thanks for posting the memory savings. 13% less memory usage
in 447.dealII is very impressive.
I haven't taken more than a very brief peek at this patch, but I
have a few questions already.
Is there a header missing? I don't see
DECLARE_TRANSPARENT_OPERAND_ACCESSORS
defined anywhere.
Also, what affect does this macro have on doxygen?
In User.h:
+public:
+ template <unsigned Idx> Use &Op() {
+ return OperandTraits<User>::op_begin(this)[Idx];
+ }
+ template <unsigned Idx> const Use &Op() const {
+ return
OperandTraits<User>::op_begin(const_cast<User*>(this))[Idx];
+ }
+ Use *allocHungoffUses(unsigned) const;
+ void dropHungoffUses(Use *U) {
+ if (OperandList == U) {
+ OperandList = 0;
+ NumOperands = 0;
+ }
+ Use::zap(U, U->getImpliedUser(), true);
+ }
At a very brief scan, it looks like allocHungoffUses and dropHungoffUses
can be made protected, not public. And maybe those Op things too.
Hmm, and why the operand index for Op is a non-type template parameter?
In an optimized build, these should be inlined, while in a non-optimized
build, having them be templates incurs instantiation overhead.
Dan
On Tue, April 29, 2008 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.
>
> Cheers,
>
> Gabor
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
On Apr 29, 6:01 pm, "Dan Gohman" <goh... at apple.com> wrote:> Hi Gabor, > > Thanks for posting the memory savings. 13% less memory usage > in 447.dealII is very impressive. > > I haven't taken more than a very brief peek at this patch, but I > have a few questions already. > > Is there a header missing? I don't see > DECLARE_TRANSPARENT_OPERAND_ACCESSORS > defined anywhere.Hi Dan, Yep, the newly added files were missing. See my other posts to this thread. My apologies, I became a victim of the tools :-)> > Also, what affect does this macro have on doxygen?Hmm, dunno, it depends whether doxigen can look thru nacros, i.e. expand them. Even if not, I think it is not too serious, because all the methods are mentioned in User, so there is no big loss if doxigen excludes them. The only exception, maybe, is the Constant* subclasses, where the return type of getOperand is covariant.> > In User.h: > > +public: > + template <unsigned Idx> Use &Op() { > + return OperandTraits<User>::op_begin(this)[Idx]; > + } > + template <unsigned Idx> const Use &Op() const { > + return OperandTraits<User>::op_begin(const_cast<User*>(this))[Idx]; > + } > + Use *allocHungoffUses(unsigned) const; > + void dropHungoffUses(Use *U) { > + if (OperandList == U) { > + OperandList = 0; > + NumOperands = 0; > + } > + Use::zap(U, U->getImpliedUser(), true); > + } > > At a very brief scan, it looks like allocHungoffUses and dropHungoffUses > can be made protected, not public. And maybe those Op things too.I have changed this on branch already.> > Hmm, and why the operand index for Op is a non-type template parameter? > In an optimized build, these should be inlined, while in a non-optimized > build, having them be templates incurs instantiation overhead.Yes, the idea was to drive the thing further and add a second, typename parameter too, so that in certain cases where the Value subclass is known for an operand N we would get an automagically casted Op<N>. This is rather vague, and I have no concrecte ideas how to implement it. I can certainly remove it. Historically, I wanted to be close to the Ops[N] syntax, Op<N>() served pretty well. What do you think of making this protected for now? Thanks for looking and asking! Cheers, Gabor> > Dan > > > > On Tue, April 29, 2008 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. > > > Cheers, > > > Gabor > > > _______________________________________________ > > LLVM Developers mailing list > > LLVM... at cs.uiuc.edu http://llvm.cs.uiuc.edu > >http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > _______________________________________________ > LLVM Developers mailing list > LLVM... at cs.uiuc.edu http://llvm.cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
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 INo 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 indentationFixed.> +++ 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.> -ChrisThanks, Chris! I shall be sending a fresh patch out soon. Cheers, Gabor