Please don't move sext/zext out of FunctionType. They need to be part of the
function type because they affect semantics. As originally intended, the 0th
parameter attribute should refer to the result of the function no the function
itself.

As for reducing size, I suggest we reserve bits in a Function for CC and
attributes. I can't imagine us ever using more than 8 (6?) bits for calling
conventions. That can leave 24 or 26 bits for boolean attributes.

Not sure about the call/invoke instructions. Can we just avoid putting them into
the instructions altogether? That's easy when the called function is directly
through a Function, not so easy when its some kind of expression.

Please don't move sext/zext out of FunctionType. They need to be part of the
function type because they affect semantics.

I agree that they affect semantics, but why does that mean they need to be part of the type? Calling
conventions affects semantics just as much and it isn't part of the type.

-Chris

True, but in this case those parameter attributes go along with the arguments.
You will wreak havoc all over the place if in one place you need a FunctionType
and in another place you need a Function. Either all the sext/zext go, or none
of them do. The problem with putting them on Function is that they consume more
space, but I'm not opposed to it.

As for making them part of the function type, it is necessary. When matching a
function call with the function types, you don't want to mismatch sext/zext on
the arguments. Hence they are part of the function type. We already tried it
the other way around (attributes on the Function) and it led to lots of issues.

True, but in this case those parameter attributes go along with the arguments.
You will wreak havoc all over the place if in one place you need a FunctionType
and in another place you need a Function. Either all the sext/zext go, or none
of them do. The problem with putting them on Function is that they consume more
space, but I'm not opposed to it.

I don't get it. I propose that the attributes are aggregated together for a function and uniqued. This
means that if you have 1000 calls to the same function which has a bunch of attributes, you just have
1000 pointers to data that tells you what the attributes need. Also, the function itself would have a
pointer to the same attribute bundle.

As for making them part of the function type, it is necessary.

No, it isn't. :)

When matching a function call with the function types, you don't want to mismatch sext/zext on
the arguments. Hence they are part of the function type.

This conclusion makes little sense to me. Yes it is bad to mismatch the arguments and the attributes.
For example, the dead arg elimination pass has to do the right thing and adjust the attributes when it
nukes an argument (as an aside, is DAE even propagating sext/zext attributes now? Yes, not doing so
will give you correct code [in this case], but it will reduce performance), however, it has to do other
things as well (e.g. update the names for arguments, which also have to be kept in sync with the
FunctionType). I don't see how keeping information in sync is different whether it's in the FunctionType
or in the Function/Call.

We already tried it the other way around (attributes on the Function) and it led to lots of issues.

What issues?

-Chris

Unfortunately, I don't remember the details. I just remember starting out the
parameter attributes feature by putting them on the function and getting into a
lot of issues with the bcreader and asmparser. IIRC, differentiating between the
short and long call syntax had something to do with it.

I didn't mean to get into a debate about this, just to warn you about my
previous experience. I would prefer that the attributes are all accessible from
one place, but I don't really care where you put them. If you split the
function attribute for sext/zext from the parameter attributes then it will be a
difficult change in the asmparser and llvm-upgrade. If you want to take on that
challenge, that's fine.

I'm still not getting it. You're apparently talking about .ll file syntax, not where they are stored in memory,
right?

I'm talking about the code in llvmAsmParser.y and UpgradeParser.y that handles
the short/long call syntax. Its not worth worrying about. Just start your
changes in those files and if it isn't problematic then I rang a false alarm. If
it is, you'll quickly discover the issue. Unfortunately, I'm still drawing a
blank on the details.

Moving function attributes out of FunctionType is orthogonal to the set
supported. Consequently, bug 1145 no longer depends on this bug. However, this
is the next thing I will tackle.

Here's what I propose:

1. Move all the attributes (parameter and function) from FunctionType to
   Function using the existing mechanism. This means that sex/zext/sret, etc.
   are not aspects of a function type, but only a function. This leads to only
   slightly more memory use than the current situation because there generally
   aren't that many more function types than functions and moving them to the
   specific functions that need the attributes may reduce the number of
   FunctionType instances anyway.

2. Don't bother with putting any attributes on a call/invoke. It isn't really
   necessary. All these attributes are basically just signals to code gen. We
   should just match parameter/return types and that's it. Is there a reason
   to add these attributes to call/invoke ?

#​1 agreed. Once the initial support is in place, we can talk about strategies for reducing memory use, etc.

#​2: you need them to support indirect function calls. If you do an indirect call to a function that takes an
i8, you need to know whether to sign or zero extend that argument when you pass it through. In the
indirect call case, you can't just look at the callee function to get the attributes. This is the same reason
that both functions and calls have calling conv info.

-Chris

Okay, so to support indirect calls, we also need to have sext/zext/inreg
(parameter attributes) associated with both the function and the call. I think
this is the reason that I originally went with FunctionType for these things
because the function type has to be specified (explicitly or implicitly) in the
call.

It might be worth splitting the notions of parameter attribute from function
attribute. For example, things like NoReturn and NoThrow could just be part of a
bit mask that also includes the calling convention. We reserve, say, 6 bits for
the CC and 10 bits for parameter attributes so it fits in 16 bits. This part
seems relatively simple and straight forward and could be done separately.

Of more concern are the parameter attributes (sext/zext/inreg) which must be
associated with each parameter. I suggest we create a new class to represent
these things since they are likely to be sparse (i.e. we won't need such an
attribute on every parameter of every function). If any are needed on a given
call or function, it is allocated and the function/call points to the object. At
some point we can unique them if necessary, which should help with programs that
call the same function bazillions of times.

For the parameter attribute class, I suggest we do something like store a
SmallVector of { i16, i16 } where the first i16 is the parameter index and the
second i16 is a set of 16 attributes that apply to that parameter. It is likely
that only a few entries will be needed for many functions so setting the
initialize size of the SmallVector to something like 2 should be good. I can't
really think of a way to compress these much more. The only other idea I've had
is to do something like encode a bitvector for each attribute type used.
Unfortunately, that has the negative side effect of making the vector big for
functions whose parameter attributes occur at the end of the parameter list.

Thoughts?

I think the goal should be to do a simple and straight-forward impl that splits them out from functiontype.
Get that working, then we can optimize it.

-Chris

Here's the increments I'll work on:

1. Extract the parameter attributes from FunctionType into their own class,
   ParamAttrsList without taking them out of FunctionType. This involves a new
   header file, ParameterAttributes.h, which is #included into very few .cpp
   files. FunctionType. just deals with pointers to ParamAttrsList.

2. Add ParamAttrsList pointers to Function and CallInst, defaulting to 0,
   without using these pointers.

3. Convert all users of FunctionType attributes to use Function/CallInst
   instead. This involves changes to AsmWriter, AsmParser, Bytecode, MSILWriter,
   SelectionDAGISel, CBE, llvm-upgrade and any other clients.

4. Unique the ParamAttrsList objects.

5. Fix comparisons of ParamAttrsList objects to use pointer equality instead
   of the class's operator==.

Step 1 is nearly done. Patch for review coming forth shortly.

Patch to implement step 1

Step 1 is done.

Step 2 is nearly done.

We're going to reverse the order of steps 3 and 4 so that uniquing is done
first. This will help with deployment to CallInst and Function.

Step 2 is done.

I don't see why it is a good idea to put attributes on all the calls and
functions instead of the function types.

It doesn't seem practical be be adding attributes like zext, inreg, or
structret after the fact, so I guess you're talking about marking
functions as noreturn or nounwind in some kind of interprocedural analysis.
These attributes can be safely bitcasted away whenever there is indirection that
can't be deciphered.

How difficult is it to create a new function type to hold the new attributes,
and then update all the uses? I don't see how making all calls memorize their
callees' parameter attributes makes anything easier; it'd still be necessary
to update each of the function's calls whenever you add new attributes.

I don't see why it is a good idea to put attributes on all the calls and
functions instead of the function types.

I assume you're objected because of the potential cost of this. In theory, the memory cost should not
be high. Once the objects are uniqued (in contrast to what I've said in the past, I now think that
uniquing does need to happen before we can switch over) the memory cost shouldn't be significant.

If we do this and find out that it is, we can reevaluate at that time.

It doesn't seem practical be be adding attributes like zext, inreg, or
structret after the fact

Right.

so I guess you're talking about marking functions as noreturn or nounwind in some
kind of interprocedural analysis.

Yes, in the future, I think there will be other (more) interesting ones as well, such as "nocapture" and
"noalias" for pointer analysis and clients. Also we can add attributes for pure/const (in the gcc
terminology) which indicates functions which only read memory or access no memory.

These attributes can be safely bitcasted away whenever there is indirection that
can't be deciphered.

I'm not sure I understand. How would this work?

How difficult is it to create a new function type to hold the new attributes,
and then update all the uses?

In the absense of indirect function calls, it is straight-forward but has cost linear with the number of
callers of the function, and has a high constant factor (because all the callinst/invokeinst instructions
would have to be deleted, then reallocated). We may be able to avoid the delete/allocation cycle, but it
could be tricky. See below for the other issue.

With indirect function calls, it gets uglier, but still doable.

I don't see how making all calls memorize their
callees' parameter attributes makes anything easier; it'd still be necessary
to update each of the function's calls whenever you add new attributes.

The issue here is due to how the LLVM type system works. An important property is that it guarantees
that pointer equality works for types. The implication of this is that types (including function types) are
immutable. Because of this, changing an attribute requires producing a new FunctionType.

Another fun design point of LLVM is that the type of a Value cannot be changed once it is created. The
implication of this is that the only way to change the type of a function is to create a new one and
update all uses of the old one to use the new one. This would require creating a new Function object,
then splicing all of the code from the old body to the new body, then updating due to new argument
bindings. This is doable and almost constant time, but it has a high constant factor.

-Chris

LangRef.html currently says "Parameter attributes are considered to be part of
the function type", and there are several implications. Are you planning to
change that as well, or have LLVM simulate it, in a sense? That's actually my
main interest here.

I think what I'm thinking of is something along the lines of
Value::replaceAllUsesWith that also changes the type of the original value to
something new; it replaces all of the uses of that value with a cast of the
value back to the original type, optionally folding away the cast in some
obvious cases. Would that be plausible within the LLVM rules?

LangRef.html currently says... Are you planning to
change that as well, or have LLVM simulate it, in a sense?

Yep, langref will be updated.

I think what I'm thinking of is something along the lines of
Value::replaceAllUsesWith that also changes the type of the original value to
something new; it replaces all of the uses of that value with a cast of the
value back to the original type, optionally folding away the cast in some
obvious cases. Would that be plausible within the LLVM rules?

Are you wondering how a specific transformation pass will be implemented? Consider pruneeh. It does
a bottom-up traversal of the callgraph and can determine that functions are noreturn or nothrow. I
would expect it to just stick the attributes onto the Function objects without worrying about all the
calls. We would then use some central pass (e.g. instcombine) to propagate these attributes from the
functions to the call/invoke instructions in the direct call case.

Does this seem reasonable?

-Chris

It seems like either way could be implemented reasonably efficiently, so I guess
just decide which way fits the language the best.

When the attributes get placed into Function/Call, don't forget to un-XFAIL
these test cases:

test/Assembler/2007-02-07-UpgradeCSRETCC.ll
test/Transforms/SimplifyCFG/2006-10-29-InvokeCrash.ll

These were xfailed to avoid temporarily fixing llvm-upgrade to deal with some
nasty csretcc cases. When the attributes are on the function/call instead of the
functiontype, these cases will be much easier to handle and llvm-upgrade can be
properly implemented.

Uniquing and reference counting of ParamAttrsList objects is now done.

Patch to complete this bug (not 100% working)

Patch for PR1146 for llvm-gcc (not 100% working)

It is unfortunate, but I wasn't able to get this working completely before the
version 2.0 branch. I have attached the latest patches for llvm and llvm-gcc.
These pass all the regression tests, but many of the tests in llvm-test are
failing. I haven't been able to track these down yet. This is what fails on Linux:

jit /External/SPEC/CFP2006/444.namd/444.namd
llc /External/SPEC/CFP2006/447.dealII/447.dealII
jit /External/SPEC/CFP2006/447.dealII/447.dealII
cbe /External/SPEC/CFP2006/447.dealII/447.dealII
cbe /External/SPEC/CINT2006/400.perlbench/400.perlbench
llc /External/SPEC/CINT2006/403.gcc/403.gcc
jit /External/SPEC/CINT2006/403.gcc/403.gcc
cbe /External/SPEC/CINT2006/403.gcc/403.gcc
llc /External/SPEC/CINT2006/458.sjeng/458.sjeng
jit /External/SPEC/CINT2006/458.sjeng/458.sjeng
cbe /External/SPEC/CINT2006/458.sjeng/458.sjeng
llc /External/SPEC/CINT2006/471.omnetpp/471.omnetpp
jit /External/SPEC/CINT2006/471.omnetpp/471.omnetpp
cbe /External/SPEC/CINT2006/471.omnetpp/471.omnetpp
cbe /External/SPEC/CINT2000/176.gcc/176.gcc
jit /External/SPEC/CINT2000/186.crafty/186.crafty
cbe /MultiSource/Applications/SPASS/SPASS
llc /MultiSource/Applications/oggenc/oggenc
jit /MultiSource/Applications/oggenc/oggenc
cbe /MultiSource/Applications/oggenc/oggenc
jit /MultiSource/Applications/minisat/minisat
jit /MultiSource/Applications/kimwitu++/kc
cbe /MultiSource/Applications/kimwitu++/kc
llc /MultiSource/Benchmarks/McCat/09-vor/vor
jit /MultiSource/Benchmarks/McCat/09-vor/vor
cbe /MultiSource/Benchmarks/McCat/09-vor/vor
llc /MultiSource/Benchmarks/Olden/power/power
jit /MultiSource/Benchmarks/Olden/power/power
cbe /MultiSource/Benchmarks/Olden/power/power
llc /MultiSource/Benchmarks/Olden/health/health
jit /MultiSource/Benchmarks/Olden/health/health
cbe /MultiSource/Benchmarks/Olden/health/health
llc /MultiSource/Benchmarks/Olden/voronoi/voronoi
jit /MultiSource/Benchmarks/Olden/voronoi/voronoi
cbe /MultiSource/Benchmarks/Olden/voronoi/voronoi
llc /MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4
jit /MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4
cbe /MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4
jit /SingleSource/UnitTests/2007-04-25-weak
llc /SingleSource/Regression/C++/EH/ctor_dtor_count-2
jit /SingleSource/Regression/C++/EH/ctor_dtor_count-2
cbe /SingleSource/Regression/C++/EH/ctor_dtor_count-2
llc /SingleSource/Regression/C++/EH/ctor_dtor_count
jit /SingleSource/Regression/C++/EH/ctor_dtor_count
cbe /SingleSource/Regression/C++/EH/ctor_dtor_count
llc /SingleSource/Regression/C++/EH/exception_spec_test
jit /SingleSource/Regression/C++/EH/exception_spec_test
cbe /SingleSource/Regression/C++/EH/exception_spec_test
llc /SingleSource/Regression/C++/EH/function_try_block
jit /SingleSource/Regression/C++/EH/function_try_block
cbe /SingleSource/Regression/C++/EH/function_try_block
llc /SingleSource/Regression/C++/EH/simple_rethrow
jit /SingleSource/Regression/C++/EH/simple_rethrow
cbe /SingleSource/Regression/C++/EH/simple_rethrow
llc /SingleSource/Regression/C++/EH/simple_throw
jit /SingleSource/Regression/C++/EH/simple_throw
cbe /SingleSource/Regression/C++/EH/simple_throw
llc /SingleSource/Regression/C++/EH/throw_rethrow_test
jit /SingleSource/Regression/C++/EH/throw_rethrow_test
cbe /SingleSource/Regression/C++/EH/throw_rethrow_test
llc /SingleSource/Regression/C++/BuiltinTypeInfo
cbe /SingleSource/Regression/C++/BuiltinTypeInfo
llc /SingleSource/Regression/C++/ofstream_ctor
cbe /SingleSource/Regression/C++/ofstream_ctor
llc /SingleSource/Benchmarks/CoyoteBench/fftbench
jit /SingleSource/Benchmarks/CoyoteBench/fftbench
cbe /SingleSource/Benchmarks/CoyoteBench/fftbench
compile /SingleSource/Benchmarks/Shootout-C++/ary2
llc /SingleSource/Benchmarks/Shootout-C++/ary2
jit /SingleSource/Benchmarks/Shootout-C++/ary2
cbe /SingleSource/Benchmarks/Shootout-C++/ary2
compile /SingleSource/Benchmarks/Shootout-C++/ary3
llc /SingleSource/Benchmarks/Shootout-C++/ary3
jit /SingleSource/Benchmarks/Shootout-C++/ary3
cbe /SingleSource/Benchmarks/Shootout-C++/ary3
compile /SingleSource/Benchmarks/Shootout-C++/ary
llc /SingleSource/Benchmarks/Shootout-C++/ary
jit /SingleSource/Benchmarks/Shootout-C++/ary
cbe /SingleSource/Benchmarks/Shootout-C++/ary
compile /SingleSource/Benchmarks/Shootout-C++/except
llc /SingleSource/Benchmarks/Shootout-C++/except
jit /SingleSource/Benchmarks/Shootout-C++/except
cbe /SingleSource/Benchmarks/Shootout-C++/except
compile /SingleSource/Benchmarks/Shootout-C++/fibo
llc /SingleSource/Benchmarks/Shootout-C++/fibo
jit /SingleSource/Benchmarks/Shootout-C++/fibo
cbe /SingleSource/Benchmarks/Shootout-C++/fibo
cbe /SingleSource/Benchmarks/Shootout-C++/hash2
llc /SingleSource/Benchmarks/Shootout-C++/hash
cbe /SingleSource/Benchmarks/Shootout-C++/hash
jit /SingleSource/Benchmarks/Shootout-C++/lists1
cbe /SingleSource/Benchmarks/Shootout-C++/spellcheck
llc /SingleSource/Benchmarks/Misc-C++/bigfib
jit /SingleSource/Benchmarks/Misc-C++/bigfib
cbe /SingleSource/Benchmarks/Misc-C++/bigfib
llc /SingleSource/CustomChecked/oopack_v1p8
jit /SingleSource/CustomChecked/oopack_v1p8
cbe /SingleSource/CustomChecked/oopack_v1p8
llc /SingleSource/CustomChecked/stepanov_v1p2
jit /SingleSource/CustomChecked/stepanov_v1p2
cbe /SingleSource/CustomChecked/stepanov_v1p2
llc /SingleSource/CustomChecked/flops
jit /SingleSource/CustomChecked/flops
cbe /SingleSource/CustomChecked/flops
llc /SingleSource/CustomChecked/paranoia
jit /SingleSource/CustomChecked/paranoia
cbe /SingleSource/CustomChecked/paranoia

FWIW, I committed these two patches to mainline, and hope they will be merged into llvm 2.0.

This should allow us to implement this feature in llvm 2.1 without having to do unnatural gymnastics in
the bitreader to be backwards compatible.

-Chris

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070507/049403.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070507/049404.html

This won't make it in 2.0 but hopefully will in 2.1

As there are several bugs depending on this one, I figure a status update is due:

I have the changes necessary for this bug to be finished but they are untested.
I believe the issue that was preventing me from committing previously has been
resolved but I won't know until I can test it fully. I almost got there on
Monday but I ran out of time. Since computers are packed and I'm in the middle
of a move, this won't get looked at again until Monday at the earliest.

it will be a welcome fix, but there is no need to get it done before monday :)

-Chris

Hi Reid, in this part (include/llvm/Transforms/IPO.h):

• bool relinkCallees = false,

•                ///< Turn external linkage for callees of function to delete
  

I don't understand the comment. What is it trying to say?

It is irrelevant to this PR, but the code in ExtractFunction.cpp says:

  // If we're in relinking mode, set linkage of all internal callees to
  // external. This will allow us extract function, and then - link
  // everything together

Reid.

llvm part ported to svn head

gcc part ported to svn head
Quick and nasty version.

It mostly works but I am seeing some testsuite failures, especially problems
with global references when bitcasting functions. For example,
Transforms/InstCombine/2007-11-25-CompatibleAttributes.ll had this:

   %tmp32 = tail call i32 (i8* noalias , ...) nounwind * bitcast (i32 (i8*, ...) nounwind * @&#8203;printf to i32 (i8* noalias , ...) nounwind *)( i8* getelementptr ([4 x i8]* @.str, i32 0, i32 0) noalias , i32 0 ) nounwind    ; <i32> [#uses=0]

I tried changing it to:

   %tmp32 = tail call i32 (i8* , ...) * bitcast (i32 (i8*, ...) * @&#8203;printf to i32 (i8* , ...) *)( i8* getelementptr ([4 x i8]* @.str, i32 0, i32 0) noalias , i32 0 ) nounwind               ; <i32> [#uses=0]

but llvm-as says:

2007-11-25-CompatibleAttributes.ll:13,0: Unresolved global references exist:
i32 (i8 *, ...) * printf

I guess that's because the @​printf used in the bitcast refers to
"declare i32 @​printf(i8*, ...)" while the actual function is
"declare i32 @​printf(i8*, ...) nounwind". This is either a bug
in the patch or I modified the testcase wrong, and there is some
way to fully specify the function including the nounwind in the
bitcast, but I don't know what it is...

Reid, can you please have a look at the new llvm patch (no need to bother
with the gcc part). I had some problems in llvmAsmParser.y in the ParamList
stuff. I mean this kind of place:

-ParamList : Types ValueRef OptParamAttrs {
+ParamList : Types OptParamAttrs ValueRef OptParamAttrs {

(I think ParamList used to be called ValuesList or something like that).
The cause of the trouble is that ParamList seems to have grown some

LABEL ValueRef OptParamAttrs

variant. I have no idea what those are, I just modified them analogously
to the other cases. I had to futz about in some other places too (I
should have noted down these places but I only thought of it after I'd
finished - sorry), but it was mostly straightforward.

Patch to llvm-gcc-4.0 module to implement PR1146
Obsolete.

new llvm part
After further investigation it seems clear that forward references to a
function cannot have parameter attributes. Thus the logic that checks
whether a function declaration has the same attributes as a forward
declaration is wrong. So I removed this logic and instead added a check
that if we see two function declarations/bodies then they have the same
attributes.

new llvm part
After further investigation it seems clear that forward references to a
function cannot have parameter attributes. Thus the logic that checks
whether a function declaration has the same attributes as a forward
declaration is wrong. So I removed this logic and instead added a check
that if we see two function declarations/bodies then they have the same
attributes.

Sorry about the duplicates, it seems to be a bugzilla bug.

The current status is that "make check" passes except for
test/Bitcode/AutoUpgradeIntrinsics.ll. The problem is that
the un-upgraded bitcode has an entry for parameter attributes
as well as the argument types. I'm talking about TYPE_CODE_FUNCTION
here. In the new bitcode there are only types, and the attributes
entry is interpreted as a type. So rather than getting
i32 (i28) you get i32 (i29, i32) and later things break. I don't
know anything about this kind of stuff so can some kind guru please
take care of it.

I ran the full testsuite with and without these patches, and there
are no additional test failures, there are only additional test
passes. They also cause no additional failures in the Ada testsuite.

That said, with the patch there are many more lines like this:
Output/2002-08-19-CodegenBug.cbe.c:113: warning: conflicting types for built-in function ‘malloc’
I suspect that this means that less code is being eliminated with
these patches - not clear why.

Also, the patches cause one "make check" failure: an auto-upgrade test
(mentioned in a previous comment).

The final bad point is that the gcc patch is rather grotty (I have some
ideas for how to clean it up though).

Nonetheless, I would like to apply the patches and sort out these
problems afterwards.

PS: Part of another bit-fiddling patch got mixed in with the gcc
part I posted. I will attach a version without this problem in a bit.

new gcc part

There is one serious problem: it doesn't autoupgrade llvm 2.1 .ll files to the new format. In fact, the parser probably dies on stuff like this:

@​FP = weak global i8 (...) signext * null

This is what llvm 2.1 produces from:

char (*FP)();

We need the parser to accept and ignore the signext in that case.

Likewise, the bc reader also needs to upgrade old bc files. This looks really easy though, just remove this hunk:

 case bitc::TYPE_CODE_FUNCTION: {

•  // FUNCTION: [vararg, attrid, retty, paramty x N]
  

•  if (Record.size() < 3)
  

•  // FUNCTION: [vararg, retty, paramty x N]
  

•  if (Record.size() < 2)
     return Error("Invalid FUNCTION type record");
   std::vector<const Type*> ArgTys;
  

•  for (unsigned i = 3, e = Record.size(); i != e; ++i)
  

•  for (unsigned i = 2, e = Record.size(); i != e; ++i)
     ArgTys.push_back(getTypeByID(Record[i], true));
  

•  ResultTy = FunctionType::get(getTypeByID(Record[2], true), ArgTys,
  

•                               Record[0], getParamAttrs(Record[1]));
  

•  ResultTy = FunctionType::get(getTypeByID(Record[1], true), ArgTys,
  

•                               Record[0]);
  

changing it to just ignore the attrid field. The bcwriter would just always write 0 for it, and hardcode this in the FunctionAbbrev abbreviation. To do this, change the abbreviation definition from:

Abbv = new BitCodeAbbrev();
Abbv->Add(BitCodeAbbrevOp(bitc::TYPE_CODE_FUNCTION));
Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isvararg

• Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed,

•                        Log2_32_Ceil(VE.getParamAttrs().size()+1)));
  

  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
  ...

to:

...
Abbv->Add(BitCodeAbbrevOp(0);
...

With this change, there is no space penalty in the new .bc files, but the old files will be transparently read.

Despite Reid's suggestion that the llvm-extract extensions are needed by the patch, I really can't see how that is. If possible, please remove them.

with the asmparser fix, many of the testsuite changes won't be needed. Once these issues are addressed, the patch looks great to me, thanks Duncan!

Fixed here (original patches by Reid Spencer, updated by yours truly):
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20071126/055824.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20071126/055825.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20071126/055826.html

mentioned in issue llvm/llvm-bugzilla-archive#1383

mentioned in issue llvm/llvm-bugzilla-archive#1393

mentioned in issue llvm/llvm-bugzilla-archive#1394

mentioned in issue llvm/llvm-bugzilla-archive#1397

mentioned in issue llvm/llvm-bugzilla-archive#1443

mentioned in issue llvm/llvm-bugzilla-archive#1583

mentioned in issue llvm/llvm-bugzilla-archive#1612

mentioned in issue llvm/llvm-bugzilla-archive#1620

mentioned in issue llvm/llvm-bugzilla-archive#1716