[llvm-commits] GC patches again

[Gordon Henriksen]

On 2007-09-27, at 14:58, Chris Lattner wrote:

>> //===-- gc-2-metadata.patch (+383) ----------------------------===//
>>
>>   include/llvm/CodeGen/CollectorMetadata.h (+198)
>>   lib/CodeGen/CollectorMetadata.cpp (+185)
>>
>> CollectorMetadata is the data structure populated by back-ends
>> during code-generation.
>>
>> This patch is independent.
>
> In CollectorMetadata.h, you can drop these two #includes:
>
> +#include <ostream>
> +#include <vector>

Done.

> they are pulled in from Pass.h.  Please reorder the remaining
> #includes as:
>
> +#include "llvm/Pass.h"
> +#include "llvm/ADT/DenseMap.h"
>
> So the ADT one comes later than the core llvm one.

Done.

> +namespace llvm {
> +
> +  class FunctionPass;
> +  class Constant;
>
> You probably don't need these.

FunctionPass deleted. I do need Constant.

> In the .cpp file, I'm not sure why you need the Deleter class.  It  
> seems like the CollectorModuleMetadata pass should manage its own  
> lifetime (freeing itself on releaseMemory).  This will require all  
> the machinefunctionpasses to preserve CollectorModuleMetadata  
> though.  Does this make sense?

Shades of <http://llvm.org/PR746>.

I thought so as well, and tried very hard to make that work. I can't  
remember the details, but I was more fundamental than just not  
invalidating the analysis. Even after I'd fixed all of the passes  
that invalidated the metadata (and there were many), I had resolve  
lifetime issues by making CollectorModuleMetadata an ImmutablePass.

I actually think this is empirically better, despite being "ugly";  
notice how you have 2 files changed here instead of 25, and no risk  
that a pass will accidentally invalidate the "analysis" and throw  
away necessary output.

— Gordon


-------------- next part --------------
A non-text attachment was scrubbed...
Name: gc-2-metadata.patch
Type: application/octet-stream
Size: 12747 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070927/981e955c/attachment.obj>