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

[Devang Patel]

On Thu, Jan 14, 2010 at 11:54 AM, Victor Hernandez <vhernandez at apple.com> wrote:
>
> 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.
>

Thanks!



-- 
-
Devang