[PATCH] use-diet for review

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

use-diet-patch-2904.diff (124 KB)

Hi Gabor, I don't see OperandTraits.h.

-Chris

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:

  //

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

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 :slight_smile:

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

> 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 :wink:
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