[llvm-commits] Fold() function on TargetFolder class - removal of redundant constantexpr

[Nick Lewycky]

Jin Gu Kang wrote:
> OK. I unstand :)
>
> I send modified patch
>
> Index: include/llvm/Support/TargetFolder.h
> ===================================================================
> --- include/llvm/Support/TargetFolder.h (revision 126080)
> +++ include/llvm/Support/TargetFolder.h (working copy)
> @@ -34,8 +34,12 @@
>     /// Fold - Fold the constant using target specific information.
>     Constant *Fold(Constant *C) const {
>       if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C))
> -      if (Constant *CF = ConstantFoldConstantExpression(CE, TD))
> +      if (Constant *CF = ConstantFoldConstantExpression(CE, TD)) {
> +        if (CF != CE)
> +         if (CE->use_empty())

Still no; just because its use-list is empty, doesn't mean that some 
piece of code doesn't have local variable "Constant *C = ..." holding 
that ConstantExpr. As strange as it sounds, constants, types and 
metadata can only be cleaned up by llvm_shutdown (or, if you're using 
llvm as a library then the creator of the LLVMContext who knows 
everything about what state llvm is in, can prove that it's safe to 
delete constants).

This isn't theoretical, either. The target folder is used by the 
IRBuilder which in turn is used by many passes to emit new IR.

Nick

PS. There's one exception, but it doesn't matter; the bitcode reader has 
a private subclass of Constant which it uses to creates placeholder 
constants. Since those can never escape the bitcode reader itself, it 
knows that it's safe to delete them.

> +           CE->destroyConstant();
>           return CF;
> +     }
>       return C;
>     }
>
>
> ________________________________________
> From: Duncan Sands [baldrick at free.fr]
> Sent: Tuesday, February 22, 2011 5:57 PM
> To: Jin Gu Kang
> Subject: Re: [llvm-commits] Fold() function on TargetFolder class - removal of redundant constantexpr
>
>> Argument of Fold() is as follows:
>> file: include/llvm/Support/TargetFolder.h on llvm-2.8
>>     Constant *CreateInBoundsGetElementPtr(Constant *C, Value* const *IdxList,
>>                                           unsigned NumIdx) const {
>>       return Fold(ConstantExpr::getInBoundsGetElementPtr(C, IdxList, NumIdx));
>>     }
>>
>> Newly made ConstantExpr is argument of Fold() function.
>
> It may not really be new, that's the point.
>
> Ciao, Duncan.
>
>> -->   ConstantExpr::getInBoundsGetElementPtr(C, IdxList, NumIdx)
>> so if Fold() function make new folded ConstantExpr, I think
>> previous created ConstantExpr is redundant.
>>
>> ________________________________________
>> From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Duncan Sands [baldrick at free.fr]
>> Sent: Tuesday, February 22, 2011 5:35 PM
>> To: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] Fold() function on TargetFolder class - removal of redundant constantexpr
>>
>> Hi Jin Gu Kang,
>>
>>> --- include/llvm/Support/TargetFolder.h (revision 126080)
>>> +++ include/llvm/Support/TargetFolder.h (working copy)
>>> @@ -34,8 +34,11 @@
>>> /// Fold - Fold the constant using target specific information.
>>> Constant *Fold(Constant *C) const {
>>> if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C))
>>> - if (Constant *CF = ConstantFoldConstantExpression(CE, TD))
>>> + if (Constant *CF = ConstantFoldConstantExpression(CE, TD)) {
>>> + if (CF != CE)
>>> + CE->destroyConstant();
>>
>> this is wrong.  Constants are global and uniqued: if someone creates a constant,
>> eg: 27, any later attempt to create an identical constant (27) returns the same
>> constant as was created earlier, not a new one.  This means that you can check
>> if constants are identical by comparing their pointer values.  It also means
>> that you can't free a constant like your patch does without first proving that
>> it is not being used anywhere.
>>
>> Ciao, Duncan.
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits