[llvm-commits] [llvm] r56704 - in /llvm/trunk: include/llvm/ lib/AsmParser/ lib/Bitcode/Reader/ lib/Transforms/IPO/ lib/Transforms/Scalar/ lib/VMCore/

[Devang Patel]

On Sep 29, 2008, at 12:25 AM, Duncan Sands wrote:

> Hi Devang,
>
>> Return attributes use 0 as the index.
>> Function attributes use ~0U as the index.
>
> attributes are stored in order of the (unsigned) index.
> Lookup traverses the list looking for the index.  So
> using ~0U means looking up function attributes may be
> particularly slow.  Since these are the attributes that
> are probably looked up most often, I suggest either
> making Index signed, so ~0U=-1 will come first, or
> special casing ~0U lookup.

This tuning, if required, can be done as a separate work.

>
>
> Ciao,
>
> Duncan.
>
>> +  //FIXME : In 3.0, stop accepting zext, sext and inreg as  
>> optional function
>> +  //attributes.
>
> Does this mean that return value attributes are now placed next
> to the return type

My next patch will do add this...

> and not at the end of the function declaration,
> but you're accepting them in both positions for the moment?

Yes, for backward compatibility.


>
>
>> -  std::vector<AttrListPtr>().swap(Attributes);
>> +  std::vector<AttrListPtr>().swap(MAttributes);
>
> What does the M stand for in MAttributes?

Not Attributes !:)

Teasing aside, M for Module. Attributes collected in this module. This  
is in bitcode library.
>
>
>> +      if (FnAttribute == Attribute::None && RetAttribute !=  
>> Attribute::None) {
>> +        if (RetAttribute & Attribute::NoUnwind) {
>> +          FnAttribute = FnAttribute | Attribute::NoUnwind;
>> +          RetAttribute = RetAttribute ^ Attribute::NoUnwind;
>> +          useUpdatedAttrs = true;
>> +        }
>> +        if (RetAttribute & Attribute::NoReturn) {
>> +          FnAttribute = FnAttribute | Attribute::NoReturn;
>> +          RetAttribute = RetAttribute ^ Attribute::NoReturn;
>> +          useUpdatedAttrs = true;
>> +        }
>> +        if (RetAttribute & Attribute::ReadOnly) {
>> +          FnAttribute = FnAttribute | Attribute::ReadOnly;
>> +          RetAttribute = RetAttribute ^ Attribute::ReadOnly;
>> +          useUpdatedAttrs = true;
>> +        }
>> +        if (RetAttribute & Attribute::ReadNone) {
>> +          FnAttribute = FnAttribute | Attribute::ReadNone;
>> +          RetAttribute = RetAttribute ^ Attribute::ReadNone;
>> +          useUpdatedAttrs = true;
>> +        }
>> +      }
>
> Rather than hard-coding this list of attributes and repeating the
> code, how about renaming
>
> const Attributes ReturnOnly = NoReturn | NoUnwind | ReadNone |  
> ReadOnly;
>
> (in Attributes.h) to FunctionAttributes, and implement the above by  
> doing
> RetAttribute & FunctionAttributes to find the list of attributes to  
> move,
> or-ing that into FnAttribute etc.

ReturnOnly will eventually support all future return attributes also,  
so it isn not suitable  here. We only want to accept existing these  
three attributes here, for backward compatibility.  We could create  
BackWordCompatibleReturnOnly but I think this is overkill. Note,  
eventually we will remove this code. However, if you feel strongly, go  
ahead.

>
>
>> +  // Add any function attributes.
>> +  if (Attributes attrs = PAL.getFnAttributes())
>> +    AttributesVec.push_back(AttributeWithIndex::get(~0, attrs));
>
> If Index becomes signed then this will need to be pushed at the front.
>
> Ciao,
>
> Duncan.

-
Devang