[llvm-commits] [llvm] r93338 - in /llvm/trunk/lib/Bitcode/Writer: ValueEnumerator.cpp ValueEnumerator.h

[Victor Hernandez]

On Jan 13, 2010, at 2:01 PM, Devang Patel wrote:

> On Wed, Jan 13, 2010 at 1:23 PM, Victor Hernandez <vhernandez at apple.com> wrote:
>> 
>> On Jan 13, 2010, at 1:00 PM, Devang Patel wrote:
>> 
>>> On Wed, Jan 13, 2010 at 11:36 AM, Victor Hernandez <vhernandez at apple.com> wrote:
>>> 
>>>> -void ValueEnumerator::EnumerateMetadata(const MetadataBase *MD) {
>>>> +void ValueEnumerator::EnumerateMetadata(const MetadataBase *MD, bool isGlobal) {
>>>>   // Check to see if it's already in!
>>>>   unsigned &MDValueID = MDValueMap[MD];
>>>>   if (MDValueID) {
>>>> @@ -237,14 +237,18 @@
>>>>   EnumerateType(MD->getType());
>>>> 
>>>>   if (const MDNode *N = dyn_cast<MDNode>(MD)) {
>>>> -    MDValues.push_back(std::make_pair(MD, 1U));
>>>> -    MDValueMap[MD] = MDValues.size();
>>>> -    MDValueID = MDValues.size();
>>>> -    for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i) {
>>>> -      if (Value *V = N->getOperand(i))
>>>> -        EnumerateValue(V);
>>>> -      else
>>>> -        EnumerateType(Type::getVoidTy(MD->getContext()));
>>>> +    if ((isGlobal && !N->isFunctionLocal()) ||
>>>> +        (!isGlobal && N->isFunctionLocal())) {
>>> 
>>> What are the cases where isGlobal does not match N->isFunctionLocal()  ?
>> 
>> Prehaps isGlobal is not the best name, it means not-during-function-incorporation.  I think I should improve that variable name.
>> 
>> The case I have encountered is:
>> 1. During ValueEnumerator creation (isGlobal = true), all metadata is iterated over, including those inside of functions, and the isFunctionLocal() metadata needs to be ignored until lazy function incorporation is done.
>> 
>> The other case should be when incorporateFunction (isGlobal = false) comes across a non-function-local metadata.  But in that case the lookup of it in MDValueMap should have succeeded and we will not reach here.
>> 
> 
> This looks like a very complex approach! I do not like adding extra
> parameter to EnumerateOperandType() just for this purpose. Is there a
> alternative ?
> 
> 
> What if you
> - ignore function local metadata while processing function args in
> ValueEnumerator construction
> - ignore non function local metadata while processing function args
> in ValueEnumerator::incorporateFunction()
> ?
> 
> This way you do not need to modify any interface.
> 
> ?

Fixed in r93446.

> -
> Devang