Function attributes are currently a property of the FunctionType. This makes it very painful to modify the attributes of a function after the function is created (because the type is immutable). It would be much better to put the attributes on the call/invoke instruction and on the Function object itself. The trick to this is to find a memory-efficient way to represent this. -Chris
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.
Created attachment 768 [details] 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.
Created attachment 832 [details] Patch to complete this bug (not 100% working)
Created attachment 833 [details] 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
This bug is code complete (has been for a while). However, there are a few nasty regressions blocking its commit. The current delta between the SVN trunk and the PR1146 changes are: > /External/SPEC/CFP2006/447.dealII/447.dealII cbe > /External/SPEC/CINT2000/252.eon/252.eon cbe > /External/SPEC/CINT2000/252.eon/252.eon jit > /External/SPEC/CINT2000/252.eon/252.eon llc > /External/SPEC/CINT2000/255.vortex/255.vortex jit > /External/SPEC/CINT2000/255.vortex/255.vortex llc > /MultiSource/Applications/kimwitu++/kc cbe < /MultiSource/Benchmarks/FreeBench/pcompress2/pcompress2 cbe > /MultiSource/Applications/spiff/spiff jit < /MultiSource/Benchmarks/McCat/18-imp/imp cbe < /MultiSource/Benchmarks/mediabench/gsm/toast/toast cbe < /MultiSource/Benchmarks/MiBench/automotive-susan/automotive-susan cbe < /MultiSource/Benchmarks/MiBench/consumer-lame/consumer-lame cbe < /MultiSource/Benchmarks/MiBench/telecomm-gsm/telecomm-gsm cbe > /SingleSource/Benchmarks/CoyoteBench/fftbench llc where > is PR1146 and < is the trunk. All attempts to bugpoint these issues have failed. More manual analysis has led to the conclusion that the LLVM changes are sound but llvm-gcc is flakey. Sometimes it generates SRET attributes, sometimes it doesn't. When this happens in large programs with many compilation units, the inconsistencies cause failures in the program. Additionally, I've seen the symptoms of bug 1612 crop up from time to time. At this point, I'm basically stuck, because I don't know enough about the intricacies of how llvm-gcc generates function prototypes via the ABI stuff and when SRET, etc. ought to be applied and when they shouldn't. Furthermore, I have not discovered why it generates them in some compilations and not in others for the same declarations. I'll provide the patches in case someone else wants to look at this.
Created attachment 1083 [details] Patch to implement PR1146 in the llvm module
Created attachment 1084 [details] Patch to llvm-gcc-4.0 module to implement PR1146
> This bug is code complete (has been for a while). However, there are a few > nasty regressions blocking its commit. Do you have a reduced testcase?
+ /// @breif Return the number of references to this ParamAttrsList breif -> brief + /// If the caller did not provide a ParamAttrsList for this call, then + /// extract them from the function, if we're calling a function. + if (!PAL) + if (const Function *F = dyn_cast<Function>(Callee)) + PAL = F->getParamAttrs(); This brings up the whole issue of what to do if the attributes of a call and the attributes of the callee differ. Since next comment. + /// FTParamAttrs - Keep track of parameter attributes that correspond to + /// FunctionTypes. The first time a function type is converted we save the + /// associated parameter attributes here for later retrieval. + std::map<const FunctionType *, const ParamAttrsList *> FTParamAttrs; This looks dangerous to me, I think it would at least be safer to map from the gcc type to the attributes. You are assuming that if two gcc types map to the same LLVM type, then both gcc types will yield the same attributes list. Is that always true? For example, it's not true if the calling conventions differ (this can't happen right now, but surely will in the future). What's more, I plan to add the nounwind attribute if the declaration is marked "nothrow". Thus the presence or not of this attribute won't depend even on the gcc type, but on the gcc function declaration. In short I don't like this approach because it will surely cause problems in the future, and may cause problems now.
Some questions about how this is supposed to work. I have two examples in mind: (1) I want to have llvm-gcc mark some calls with the nounwind attribute. The called function itself won't in general have this attribute set. This is needed to handle "no-throw regions" which enforce for example the C++ requirement that destructors don't propagate exceptions. (2) Presumably it is intended that if the optimizers discover something about a function, then they can adjust the function's attributes. For example, it would be easy to determine that some functions can't throw exceptions by inspecting the function body (-> set nounwind attribute) or can't return (-> set noreturn attribute). Questions: (a) What to do when call and callee have different attributes? In the case of nounwind, users want to know when either the call or the callee have the attribute, so at least there should be utility functions for determining this. Similarly for example (2). Or maybe calls should be required to have *at least* the attributes of the callee. (b) What to do when the optimizers resolve an indirect function call into a direct function call? In this case it is important that call attributes such as nounwind don't get lost. Presumably the call should get the "or" of the call and callee attributes. Also, it may be determined that the call and the callee have incompatible attributes (see MutuallyIncompatible in the verifier), in which case presumably the call should be lowered to undef or something like that. (c) Function resolution (linking). Suppose while linking two modules we discover that we have two functions with the same name and the same type but different attributes. Are they considered to be the same function? Should attributes be "or'd" together? For example, llvm-g++ knows that "exit" cannot unwind, so in the future we may want to mark the exit function with the nounwind attribute. However a declaration of "exit" in a module compiled from C won't have this attribute. During linking we should understand that these two functions are the same, and that we want to keep the nounwind attribute. On the other hand, if we just "or" attributes together we may end up with illegal combinations, see MutuallyIncompatible.
In fact it is already the case that parameter attributes depend on (1) the gcc type, (2) the gcc declaration and (3) the call. For example, the gcc declaration is used to set the NoAlias attribute, while the call itself is used to set the Nest attribute (via the static_chain argument, which is sometimes extracted from the call).
On the subject of attributes, what's the point of having them be uniqued? I don't think it makes much sense to compare the entire collection of attributes for two functions, which is what uniquing is good for. Wouldn't it make more sense to put the attribute in the Argument class?
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.
Created attachment 1265 [details] llvm part ported to svn head
Created attachment 1266 [details] 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 * @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*, ...) * @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.
Comment on attachment 1084 [details] Patch to llvm-gcc-4.0 module to implement PR1146 Obsolete.
Created attachment 1268 [details] 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.
Created attachment 1269 [details] 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.
Created attachment 1270 [details] 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