[llvm-commits] [poolalloc] r106874 - in /poolalloc/trunk: include/poolalloc/PoolAllocate.h lib/PoolAllocate/PoolAllocate.cpp
John Criswell
criswell at uiuc.edu
Fri Jun 25 12:33:13 PDT 2010
Author: criswell
Date: Fri Jun 25 14:33:13 2010
New Revision: 106874
URL: http://llvm.org/viewvc/llvm-project?rev=106874&view=rev
Log:
Added FIXME comments from the first code review.
Modified:
poolalloc/trunk/include/poolalloc/PoolAllocate.h
poolalloc/trunk/lib/PoolAllocate/PoolAllocate.cpp
Modified: poolalloc/trunk/include/poolalloc/PoolAllocate.h
URL: http://llvm.org/viewvc/llvm-project/poolalloc/trunk/include/poolalloc/PoolAllocate.h?rev=106874&r1=106873&r2=106874&view=diff
==============================================================================
--- poolalloc/trunk/include/poolalloc/PoolAllocate.h (original)
+++ poolalloc/trunk/include/poolalloc/PoolAllocate.h Fri Jun 25 14:33:13 2010
@@ -53,12 +53,19 @@
/// case, the Clone and NewToOldValueMap information identify how the clone
/// maps to the original function...
///
+ /// FIXME: This structure should implement information hiding. There should
+ /// be some information hiding. The design may depend on how we change
+ /// the way in which clients interface with pool allocation analysis
+ /// results.
+ ///
struct FuncInfo {
FuncInfo(Function &f) : F(f), Clone(0), rev_pool_desc_map_computed(false) {}
/// MarkedNodes - The set of nodes which are not locally pool allocatable in
/// the current function.
///
+ /// FIXME: This field has a non-descriptive name.
+ ///
DenseSet<const DSNode*> MarkedNodes;
/// F - The function this FuncInfo corresponds to.
@@ -83,12 +90,13 @@
/// function.
std::map<const DSNode*, Value*> PoolDescriptors;
- //Reverse mapping for PoolDescriptors, needed by TPPA
+ // Reverse mapping for PoolDescriptors, needed by TPPA
+ // FIXME: There can be multiple DSNodes mapped to a single pool descriptor
std::map<Value*, const DSNode*> ReversePoolDescriptors;
- //This is a hack -- a function should be added which maintains these in parallel
- //and all of PoolAlloc and SafeCode should be updated to use it instead of adding
- //to either map directly.
+ // This is a hack -- a function should be added which maintains these in parallel
+ // and all of PoolAlloc and SafeCode should be updated to use it instead of adding
+ // to either map directly.
bool rev_pool_desc_map_computed;
void calculate_reverse_pool_descriptors()
{
@@ -102,6 +110,8 @@
/// This is a map from Old to New Values (the reverse of NewToOldValueMap).
/// SAFECode uses this for check insertion.
+ /// FIXME: Does SAFECode still use this? Should a single class handle this map and the
+ /// NewToOldValue Map?
std::map<const Value*, Value*> ValueMap;
/// NewToOldValueMap - When and if a function needs to be cloned, this map
@@ -133,12 +143,17 @@
public:
static char ID;
+ // FIXME: Some of these things should not be public.
Constant *PoolRegister;
+
+ // FIXME: We want to remove SAFECode specified flags.
bool SAFECodeEnabled;
bool BoundsChecksEnabled;
enum LIE_TYPE {LIE_NONE, LIE_PRESERVE_DSA, LIE_PRESERVE_ALL, LIE_PRESERVE_DEFAULT};
+ // FIXME: Try to minimize lying
LIE_TYPE lie_preserve_passes;
+ // FIXME: Let clients choose which DSA pass is used by scheduling it ahead of time.
enum PASS_TYPE {PASS_EQTD, PASS_BUEQ, PASS_DEFAULT};
PASS_TYPE dsa_pass_to_use;
@@ -150,6 +165,7 @@
virtual PA::FuncInfo *getFuncInfoOrClone(const Function &F) {return 0;}
virtual Function *getOrigFunctionFromClone(const Function *F) const {return 0;}
+ // FIXME: Clients should be able to specialize pool descriptor type
virtual const Type * getPoolType(LLVMContext*) {return 0;}
virtual bool hasDSGraph (const Function & F) const {
@@ -164,7 +180,11 @@
return Graphs->getGlobalsGraph ();
}
- /* Return value is of type PoolDescPtrTy */
+ // Return value is of type PoolDescPtrTy
+ // FIXME: Do we need to have a getGlobalPool() for clients?
+ // FIXME: Can we infer the function? Possibly not since we want to distinguish between
+ // pools in clones and pools in original function.
+ // FIXME: We want something like getPool (Value *) if possible.
virtual Value * getPool (const DSNode * N, Function & F) {return 0;}
virtual Value * getGlobalPool (const DSNode * Node) {return 0;}
@@ -181,8 +201,11 @@
bool PassAllArguments;
Module *CurModule;
+
+ // FIXME: Where is this used? Why isn't DSCallGraph used directly?
CallTargetFinder* CTF;
+ // Map a cloned function to its original function
std::map<const Function*, Function*> CloneToOrigMap;
public:
@@ -243,6 +266,7 @@
virtual void getAnalysisUsage(AnalysisUsage &AU) const;
+ // FIXME: This method is misnamed.
DataStructures &getGraphs() const { return *Graphs; }
/// getOrigFunctionFromClone - Given a pointer to a function that was cloned
@@ -253,6 +277,8 @@
return I != CloneToOrigMap.end() ? I->second : 0;
}
+ // FIXME: add isClone()
+
/// getFuncInfo - Return the FuncInfo object for the specified function.
///
PA::FuncInfo *getFuncInfo(const Function &F) {
@@ -291,6 +317,7 @@
Instruction *IPHint = 0);
/// getPoolType - Return the type of a pool descriptor
+ /// FIXME: These constants should be chosen by the client
const Type * getPoolType(LLVMContext* C) {
const IntegerType * IT = IntegerType::getInt8Ty(*C);
Type * VoidPtrType = PointerType::getUnqual(IT);
@@ -326,7 +353,7 @@
//
virtual Value * getPool (const DSNode * N, Function & F) {
//
- // Grab the structure containg information about the function and its
+ // Grab the structure containing information about the function and its
// clones.
//
PA::FuncInfo * FI = getFuncInfoOrClone (F);
@@ -519,6 +546,7 @@
/// It requires some work on code clean up to make these two pass integrate
/// nicely.
+// FIXME: Is this used? Should it be removed?
class PoolAllocateMultipleGlobalPool : public PoolAllocate {
TargetData * TD;
void ProcessFunctionBodySimple(Function& F, TargetData & TD);
Modified: poolalloc/trunk/lib/PoolAllocate/PoolAllocate.cpp
URL: http://llvm.org/viewvc/llvm-project/poolalloc/trunk/lib/PoolAllocate/PoolAllocate.cpp?rev=106874&r1=106873&r2=106874&view=diff
==============================================================================
--- poolalloc/trunk/lib/PoolAllocate/PoolAllocate.cpp (original)
+++ poolalloc/trunk/lib/PoolAllocate/PoolAllocate.cpp Fri Jun 25 14:33:13 2010
@@ -114,6 +114,8 @@
// DSA. For Automatic Pool Allocation only, we need Bottom-Up DSA. In all
// cases, we need to use the Equivalence-Class version of DSA.
//
+ // FIXME: Is the PASS_DEFAULT value used?
+ //
if (dsa_pass_to_use == PASS_EQTD)
Graphs = &getAnalysis<EQTDDataStructures>();
else
@@ -145,6 +147,7 @@
// arguments, make its clone.
//
// FIXME: Can the code below invalidate the function iterator?
+ // FIXME: Should use a isClone() method.
//
std::set<Function*> ClonedFunctions;
for (Module::iterator I = M.begin(), E = M.end(); I != E; ++I)
@@ -156,7 +159,9 @@
}
// Now that all call targets are available, rewrite the function bodies of the
- // clones.
+ // clones or the original function (if the original has no clone).
+ //
+ // FIXME: Use utility methods to make this code more readable!
for (Module::iterator I = M.begin(), E = M.end(); I != E; ++I)
if (!I->isDeclaration() && !ClonedFunctions.count(I) &&
Graphs->hasDSGraph(*I)) {
@@ -400,9 +405,11 @@
// We also need to pass in pools for any value that is reachable via a
// function argument.
//
- // Of course, skip this is this function is the main() function. We can't
+ // Of course, skip this if this function is the main() function. We can't
// really add pools to main(). :)
//
+ // FIXME: This needs to handle varargs properly.
+ //
if (F.getName() != "main") {
// All DSNodes reachable from arguments must be passed in.
for (Function::arg_iterator I = F.arg_begin(), E = F.arg_end();
@@ -437,6 +444,8 @@
// FIXME:
// 1) PassAllArguments seems to be ignored here. Why is that?
// 2) Why is the heap node check part of the PassAllArguments check?
+ // 3) SAFECode probably needs to pass the pool even if it's not a heap node.
+ // We should probably just do what the heuristic tells us to do.
//
for (DenseSet<const DSNode*>::iterator I = MarkedNodes.begin(),
E = MarkedNodes.end(); I != E; ) {
@@ -575,10 +584,16 @@
return FI.Clone = New;
}
+//
+// FIXME: Update comment
+//
+// FIXME: Global pools should probably be initialized by a global ctor instead of by
+// main().
+//
// SetupGlobalPools - Create global pools for all DSNodes in the globals graph
// which contain heap objects. If a global variable points to a piece of memory
// allocated from the heap, this pool gets a global lifetime. This is
-// implemented by making the pool descriptor be a global variable of it's own,
+// implemented by making the pool descriptor be a global variable of its own,
// and initializing the pool on entrance to main. Note that we never destroy
// the pool, because it has global lifetime.
//
@@ -599,6 +614,10 @@
E = GlobalHeapNodes.end(); I != E; ) {
DenseSet<const DSNode*>::iterator Last = I; ++I;
+ //
+ // FIXME: If the PoolAllocateAllGlobalNodes option is selected for the heuristic,
+ // then we should make global pools for heap and non-heap DSNodes.
+ //
#if 0
//
// FIXME:
@@ -632,7 +651,7 @@
//std::vector<const DSNode*> NodesToPA(GlobalHeapNodes.begin(),
// GlobalHeapNodes.end());
- //DenseSet Doesn't have polite iterators
+ //FIXME: Explain in more detail: DenseSet Doesn't have polite iterators
std::vector<const DSNode*> NodesToPA;
for (DenseSet<const DSNode*>::iterator ii = GlobalHeapNodes.begin(),
ee = GlobalHeapNodes.end(); ii != ee; ++ii)
More information about the llvm-commits
mailing list