[llvm-commits] [llvm] r47342 - in /llvm/trunk: include/llvm/ include/llvm/Support/ lib/AsmParser/ lib/Bitcode/Writer/ lib/Transforms/IPO/ lib/Transforms/Scalar/ lib/VMCore/ tools/llvm2cpp/

[Chris Lattner]

On Feb 20, 2008, at 10:09 AM, Dale Johannesen wrote:

>
> On Feb 19, 2008, at 8:04 PM, Chris Lattner wrote:
>> First, the ParamAttrsWithIndex looks like it needs to grow the  
>> 'attrs'
>> member to 32-bits.
>
> I did; but I can't figure out if you misread the patch or meant
> something else, please look again.

You're right, sorry!

>> other headers only include the former "light" header, and only .cpp
>> files #include the full header.  Is this reasonable?
>
> It seems natural to me to keep all the types that manipulate
> ParamAttrs together, and I really doubt there's a noticeable
> performance issue.  I guess I can do this, but why do you want to?

The general idea is to keep coupling of the various components in LLVM  
as loose as possible.  There are a variety of ways to do this, and C++  
is not a perfect vehicle for this (it would be really nice to have  
forward declared enums for example).  However, the general policy is:

1. #include as little as possible in headers, which forces .cpp files  
to #include what they need.
2. .cpp files should #include as little as possible.
3. Factor headers so that they different clients which need different  
subsets only pull in what they need.
4. Separate headers from their implementation files, so that libraries  
can have private headers... while keeping the distinction between  
public and private headers clear.
5. Make the include hierarchy reflect the logical library structure.

None of these are absolute hard requirements or rules, they are more  
of guiding principles.  In this case, pulling in FoldingSet.h into  
Instruction.h means that just about everything that manipulates the IR  
will get it.  This means that .cpp files should have their #include  
removed (by #2) and it thus makes it less obvious what is happening.

>>> +const Attributes Alignment = 0xffff<<16; ///< Alignment of
>>> parameter (16 bits)
>>> +                                    // 0 = unknown, else in clear
>>> (not log)
>>
>> 16 bits is a lot of bits for an alignment.  How about 4 bits stored  
>> in
>> log form, where 0 -> alignment unspecified, and 1-15 mean "1 <<
>> (n-1)"?  This gives us lots of bits for other crazy param attrs in  
>> the
>> future.
>
> There are the same number of free bits that there were before my
> change.  If I'd wanted to use a log representation I could have
> squeezed the alignment into the existing 16 bits; this way is easier
> to read and more efficient to access.  (And as a theoretical matter,
> alignments need not be powers of 2, although I don't know of a still-
> live target where that would be useful.)

I don't think efficiency of access is a big deal here, these are very  
infrequently accessed.  I'd like to keep the representation dense.

-Chris