Bugzilla – Bug 122
[code-cleanup] SymbolTable class cleanup, Type should not derive from Value, eliminate ConstantPointerRef class
Last modified: 2004-07-17 20:19:26
You need to log in before you can comment on or make changes to this bug.
It would be nice if the following changes were made to the core LLVM class hierarchy: 1. Type should not derive from Value. Types are not values. Originally they were made this way to make the symbol table easier to implemented, but that's the wrong reason. 2. The SymbolTable class should not derive from std::map, it should _contain_ an std::map, which will allow us to simplify and understand the interface. 3. The ConstantPointerRef bridge class should be eliminated. To do this, we need to make GlobalValue derive from Constant, which it should have done all along. ConstantPointerRef's are an ugly wart that needs to be removed from LLVM. -Chris
Also, the alloca and malloc instructions should take an optional alignment argument. This argument, for malloc, allows us to represent valloc and memalign directly in LLVM. For alloca, it allows us to support the alignment attributes in the GCC front-end directly. Global variables should also be extended to support alignment attributes. This will be required for vectorization work. If an alignment is specified as zero, the default, then that means we should follow the target alignment settings. This should be _by far_ the most common setting, so we should not even bother encoding this argument into the bytecode file unless it is nonzero. -Chris
Changing all of these bugs who do not have people looking at them to be assigned to "unassignedbugs", indicating that if someone is feeling ambitious, they can take ownership of the bug. If I stole your bug, and you still want it, feel free to take ownership back. -Chris
Mine. In an effort to learn more about VMCore, I'm going to try and implement this bug. I'm going to start with the SymbolTable since I'm familiar with it and the change is relatively straightforward. If I can do that without breaking things, I'll try making Type not derive from Value. Each change will constitute a separate patch on a CVS branch.
Nice! Sounds great! As you probably know, the best way to go about this is to add methods to SymbolTable that are *only* used for dealing with types, and force all of LLVM to go through that interface (ie, the existing interface would assert if given a type). Once everything is converted over, we can see what breaks when we make Type not derive from Value. That will be a big win, both for making LLVM cleaner and for reducing the memory consumption of allocated types (which there can be a lot of). As always, let me know if you have any questions or run into any problems. :) -Chris
That's pretty much the tack I've taken. I've started with making the std::map base type of SymbolTable be a data member. That work is already done and I've converted all the code in VMCore that directly manipulated the base class with functors on the interface of SymbolTable. I'm going to do this in a very slow, careful way. Once I've got the SymbolTable change to compile, I'll make sure all the regressions still work. When they do, I'll head into the Value != Type change.
Work on this bug is being committed to branch bug_122. Currently, I have completed step 2, making SymbolTable not derive from std::map. After this change, the regression test looks like: --- STATISTICS --------------------------------------------------------------- 336 tests total 328 ( 98%) tests as expected 1 ( 0%) tests unexpected ERROR 7 ( 2%) tests unexpected FAIL --- TESTS WITH UNEXPECTED OUTCOMES ------------------------------------------- Regression.Assembler.ConstantExprFold : FAIL , expected PASS Script: /proj/work/llvm/build/test/tmp/trConstantExprFold.llx/testscript.ConstantExprFold.llx Output: /proj/work/llvm/build/test/tmp/trConstantExprFold.llx/testscript.ConstantExprFold.llx.out Regression.C++Frontend.2004-01-11-DynamicInitializedConstant: FAIL , expected PASS Script: /proj/work/llvm/build/test/tmp/tr2004-01-11-DynamicInitializedConstant.cpp.tr/testscript.2004-01-11-DynamicInitializedConstant.cpp.tr Output: /proj/work/llvm/build/test/tmp/tr2004-01-11-DynamicInitializedConstant.cpp.tr/testscript.2004-01-11-DynamicInitializedConstant.cpp.tr.out Regression.C++Frontend.2004-03-08-ReinterpretCastCopy: FAIL , expected PASS Compiling C++ code failed Regression.C++Frontend.2004-03-15-CleanupsAndGotos: FAIL , expected PASS Compiling C++ code failed Regression.CFrontend.2003-06-29-MultipleFunctionDefinition: FAIL , expected PASS Compiling C code failed Regression.CFrontend.2003-12-14-ExternInlineSupport: FAIL , expected PASS Script: /proj/work/llvm/build/test/tmp/tr2003-12-14-ExternInlineSupport.c.tr/testscript.2003-12-14-ExternInlineSupport.c.tr Output: /proj/work/llvm/build/test/tmp/tr2003-12-14-ExternInlineSupport.c.tr/testscript.2003-12-14-ExternInlineSupport.c.tr.out Regression.CFrontend.2004-01-08-ExternInlineRedefine: FAIL , expected PASS Compiling C code failed Regression.CFrontend.2004-02-12-LargeAggregateCopy: ERROR , expected PASS Interrupted. make: [qmtest] Error 1 (ignored)
The SymbolTable redesign is completed. I have also committed changes to reduce the use of Type::TypeTy which is slated to disappear with the dissociation of Type from Value. While I complete this bug, and until Type::TypeTy can go away, please do not propagate new code that uses it. There is no reason to use it in order to access the "type plane" in the new SymbolTable. There are some corner cases where its still needed elsewhere, however. Here's a quickie summary of how the new SymbolTable interface affects its users: 1. The data structures are embedded so if you were thinking of using any std::map functionality, its gone. 2. Some of the generic std::map functionality has been added to SymbolTable to make it compatible (e.g. empty() ). 3. The iterator names and concepts have changed significantly. In the future, please use the following conventions: (a) To iterate over the all type planes use plane_begin() and plane_end(). These are semantically equivalent to the begin() and end() methods inherited from std::map on the old SymbolTable. For iterating over planes, please use an iterator named PI. (b) To iterate over the type type plane (the plane of Type::TypeTy), please use type_begin() and type_end(). Previously, users of the SymbolTable would use ST.find(Type::TypeTy) to access the type type plane. This is now semantically incorrect and will always return std::map.end()! Use type_begin() and type_end() instead. These will get you an iterator over the name/Type pairs in the type type plane. For iteration over types, please use an iterator named TI. (c) To iterate over the values in one type plane (except for the plane of types), use value_begin(Type*) and value_end(Type*). These methods will get you an iterator over the name/Value pairs in the single type plane given by the arguments. For iterating over values, please use an iterator named VI. 4. Some functionality (like "strip") has been moved into the SymbolTable. This means that the SymbolStripping pass is now a single call to the SymbolTable::strip() method. Next steps on bug 122 (each a separate commit): * Completely rid ourselves of Type::TypeTy. This involves a rewrite of the SlotCalculator which will be divided up between Bytecode writer and ASM writer. * Make Type not inherit from Value and fix the resulting fall out. * Get rid of ConstantPointerRef.
An incremental step has been committed to CVS. I've separated the AsmWriter use of SlotCalculator from the Bytecode Writer's use of it. AsmWriter now has a "SlotMachine" embedded into the anonymous namespace in AsmWriter.cpp that is a drastically pared down version of SlotCalculator. Additionally, the checks in SlotCalculator to see if we're writing Bytecode or not have been removed since SlotCalculator is now only used by Bytecode Writer.
Another cleanup when Type does not derive from Value: remove "setName" in the Type class. Everyone should go through the SymbolTable or Module interfaces to set names *for* types, because you can't set a name *on* a type! -Chris
The Type != Value change has been committed. Here are the patches: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015673.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015674.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015675.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015676.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015677.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015678.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015679.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015680.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015681.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015682.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015683.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015684.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015685.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015686.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015687.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015689.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015690.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015691.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015692.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015693.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015694.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015695.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015696.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015697.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015698.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015699.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015700.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015701.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015702.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040628/015703.html
As a note, the stuff in Comment #1 has now been turned into Bug 400. I believe the CPR changes are the only thing left in this bug. -Chris
Created an attachment (id=153) [details] CPR Elimination Discussion From #llvm IRC This attachment provides a discussion between Chris and Reid about how to make getting rid of ConstantPointerRef easier.
The only remaining task before closing this bug is to remove ConstantPointerRef from the LLVM IR. This will happen by making GlobalValue derive from Constant and adjusting the use of Constants. However, as described by Chris in the first attachment, there are some ways to do this that will make things easier. The remaining tasks consist of: 1. Make the GlobalValue destructor automatically delete any Constants that are pointing to the GlobalValue. The basic logic is in the Constant::destroyConstant() method. This should traverse the constant graph to make sure any dead constants it uses are also deleted. If the GlobalValue itself is used then an assertion should happen because attempting to delete used GlobalValues is an IR error. 2. Provide a GlobalValue method similar to use_empty (say, use_empty_except_constants) that will return true if the only users of the GlobalValue are transitively dead constants. 3. Remove the ConstantPointerRef class and make GlobalValue derive from Constant. Note that doing 1 and 2 first will make 3 much easier because much of the CPR code will be eliminated that would have otherwise needed to be modified.
As a further "incrementalization" of #3, I'll point out that you can make GlobalValue derive from constant without necessarily auditing all of the isa<Constant>/isa<GlobalValue> calls. I'm not sure if that will actually help, because the work must be done, but fine-grained incrementalization is good :) -Chris
There were only 9 of those that I found with llvmgrep so they're done.
The CPR Changes are done. Commits are here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040712/016108.html through http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20040712/016170.html
The changes required for this bug have been completed. Marking it resolved.
One more thing: MAJOR KUDOS to Chris Lattner for his expert technical assistance in the resolution of this bug. It was a real education for me, which was my main interest in tackling this bug.