Hi all, It's been a long time, and I'm probably going to kill myself, but I want to try it anyway. Bug 10368 [1] tells me that ConstantExpr shouldn't automatically fold, and that this is source of many problems (most notably with traps) and code duplication. However, I'm a bit lost... There seem to be constant folding in many places like ConstantExpr::get*() uses lib/Analysis/ConstantFolding.cpp, that uses lib/VMCore/ConstantFold.cpp InstCombine also has some instruction simplification (duh), but it mostly deal with variables, so it only returns a semantically and simpler equivalent operation, rather than go all the way to fold if both arguments are constants (I guess). So, is the idea to transform ConstantFolding into a new function pass, or to recode another pass (like InstCombine) to deal with such things? -- cheers, --renato http://systemcall.org/ [1] http://llvm.org/bugs/show_bug.cgi?id=10368
On Fri, Jun 29, 2012 at 3:10 PM, Renato Golin <rengolin at systemcall.org> wrote:> Hi all, > > It's been a long time, and I'm probably going to kill myself, but I > want to try it anyway. > > Bug 10368 [1] tells me that ConstantExpr shouldn't automatically fold, > and that this is source of many problems (most notably with traps) and > code duplication. > > However, I'm a bit lost... There seem to be constant folding in many places like > > ConstantExpr::get*() uses > lib/Analysis/ConstantFolding.cpp, that uses > lib/VMCore/ConstantFold.cpp > > InstCombine also has some instruction simplification (duh), but it > mostly deal with variables, so it only returns a semantically and > simpler equivalent operation, rather than go all the way to fold if > both arguments are constants (I guess). > > So, is the idea to transform ConstantFolding into a new function pass, > or to recode another pass (like InstCombine) to deal with such things?No new passes; the idea is that all constant folding should be done by InstSimplify. -Eli
On Fri, 29 Jun 2012 23:10:39 +0100 Renato Golin <rengolin at systemcall.org> wrote:> Hi all, > > It's been a long time, and I'm probably going to kill myself, but I > want to try it anyway. > > Bug 10368 [1] tells me that ConstantExpr shouldn't automatically fold, > and that this is source of many problems (most notably with traps) and > code duplication. > > However, I'm a bit lost... There seem to be constant folding in many > places like > > ConstantExpr::get*() uses > lib/Analysis/ConstantFolding.cpp, that uses > lib/VMCore/ConstantFold.cpp > > InstCombine also has some instruction simplification (duh), but it > mostly deal with variables, so it only returns a semantically and > simpler equivalent operation, rather than go all the way to fold if > both arguments are constants (I guess). > > So, is the idea to transform ConstantFolding into a new function pass, > or to recode another pass (like InstCombine) to deal with such things? > >This seems like yet-another place where target-information integration would be helpful (and, indeed, should be used). -Hal -- Hal Finkel Postdoctoral Appointee Leadership Computing Facility Argonne National Laboratory
On 29 June 2012 23:34, Eli Friedman <eli.friedman at gmail.com> wrote:> No new passes; the idea is that all constant folding should be done by > InstSimplify.Ah, found it! ;) Ok, that seems simple enough... but there is a problem. If we remove the folding from ConstantExpr, lots of things will break (and I don't trust the tests to tell me all of them), so my idea was to replace it with InstSimplify (at build time), and slowly remove it from each get*() function and fix the bugs. But that means I'll have to call a FunctionPass at build time, and I'm not sure that's possible, or even desirable. Is there a better alternative? -- cheers, --renato http://systemcall.org/
On 29 June 2012 23:40, Hal Finkel <hfinkel at anl.gov> wrote:> This seems like yet-another place where target-information integration > would be helpful (and, indeed, should be used).Indeed! And it's part of the plan, to make sure we get it right. However, since all folding will eventually be moved to the function pass, that's gotta be coded (if not there yet) on the pass. I haven't looked all of ti yet, but I guess there is already some support for it, like other passes. I think that's the whole point. Instead of coding traps and target info on constant folding, we should keep them original until the proper pass comes and does it right. -- cheers, --renato http://systemcall.org/
Renato Golin wrote:> Hi all, > > It's been a long time, and I'm probably going to kill myself, but I > want to try it anyway.I don't think that turning off folding of constants is the right place to start. To implement this, start by adding new constants that are going to replace the existing ones. A good rule of thumb is "whatever the relocations in a given object format support", and the start making llvm support them. Given that it's driven by object formats, these new nodes will only be created by something that has a TargetData object. Maybe its API takes an existing instruction to fold, and it may or may not produce a constant depending on the target. Or start snipping off individual ConstantExpr nodes. Go through all of llvm and remove "shufflevector" constant expression, make sure it's never formed. Anyone who tries to constant fold it should use the new methods you created in my previous paragraph, and it should only return NULL and stay as instructions or produce a new constant vector, but no shufflevector ConstantExpr. Insert/Extract element/value are next, then possible select or fcmp. Eventually we'll get down to things like "subtract" and that should be a target specific node, as I don't think all .o formats can actually implement that. One other request that isn't in the PR, I'd like whatever replaces GEP to not store "i32 0" vs. "i64 0". Right now "i8* getelementptr ([1 x i8]* @glbl), i32 0, i32 0" is different from "i8* getelement ([1 x i8]* @glbl), i33 0, i33 0", and there's an infinite number of these. We should canonicalize these harder and only produce one Value* here. Nick> > Bug 10368 [1] tells me that ConstantExpr shouldn't automatically fold, > and that this is source of many problems (most notably with traps) and > code duplication. > > However, I'm a bit lost... There seem to be constant folding in many places like > > ConstantExpr::get*() uses > lib/Analysis/ConstantFolding.cpp, that uses > lib/VMCore/ConstantFold.cpp > > InstCombine also has some instruction simplification (duh), but it > mostly deal with variables, so it only returns a semantically and > simpler equivalent operation, rather than go all the way to fold if > both arguments are constants (I guess). > > So, is the idea to transform ConstantFolding into a new function pass, > or to recode another pass (like InstCombine) to deal with such things?
On 1 July 2012 01:35, Nick Lewycky <nicholas at mxc.ca> wrote:> I don't think that turning off folding of constants is the right place to > start.Me neither!> To implement this, start by adding new constants that are going to > replace the existing ones.Adding what, where? I can't add a new ConstantInt if the old one is still there. If the new unfolding Int gets used, it'll change the behaviour.> Or start snipping off individual ConstantExpr nodes. Go through all of llvm > and remove "shufflevector" constant expression, make sure it's never formed. > Anyone who tries to constant fold it should use the new methods you created > in my previous paragraph, and it should only return NULL and stay as > instructions or produce a new constant vector, but no shufflevector > ConstantExpr. Insert/Extract element/value are next, then possible select or > fcmp. Eventually we'll get down to things like "subtract" and that should be > a target specific node, as I don't think all .o formats can actually > implement that.That's another approach, and one that could be more feasible. Instead of backing up the folding with the pass, I could just turn off each one individually and deal with one problem at a time. That'll take a long time, but I guess it's the best option...> One other request that isn't in the PR, I'd like whatever replaces GEP to > not store "i32 0" vs. "i64 0". Right now "i8* getelementptr ([1 x i8]* > @glbl), i32 0, i32 0" is different from "i8* getelement ([1 x i8]* @glbl), > i33 0, i33 0", and there's an infinite number of these. We should > canonicalize these harder and only produce one Value* here.That's kinda silly, I agree. Maybe I should start with this one, which seams simpler, than later tackle the rest. After I finish it, we could make a list of the most painful ones, so I can start with them. -- cheers, --renato http://systemcall.org/