ConstantExpr refactoring

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

This seems like yet-another place where target-information integration
would be helpful (and, indeed, should be used).

-Hal

Ah, found it! :wink:

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?

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.

> 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 this depends on when you want this pass to execute. The only
IR-level pass for which this is true is LSR -- even though it is an
IR-level pass, it takes a TLI instance, and lives in llc.
Alternatively, you could make it into a CodeGen-level (SelectionDAG)
pass.

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.

So the general idea here is to rely on SCEV to allow passes to evaluate
constant values but otherwise leave them uncollapsed?

Thanks again,
Hal

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

I think this depends on when you want this pass to execute.

I'd like to mimic the current behaviour using the pass, for now.
Later, slowly turning off the features, one by one, and letting the
pass deal with folding (during codegen).

So the general idea here is to rely on SCEV to allow passes to evaluate
constant values but otherwise leave them uncollapsed?

Yes.

--renato

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.