[llvm] r209007 - Fix most of PR10367.

[Nick Lewycky]

John McCall wrote:
> On May 17, 2014, at 9:03 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>> John McCall wrote:
>>> On May 17, 2014, at 7:54 PM, Rafael Espíndola<rafael.espindola at gmail.com>   wrote:
>>>>> That argument works when it’s really just “allowing two different types”, but it breaks down the more features you add to aliases.  If aliases support bitcasts (by just allowing the type to be different) and geps (by allowing an offset to be given) and address-space conversions (by allowing an explicit address space that doesn’t match the base)
>>>>
>>>> That is part of the type.
>>>>
>>>>> then it sure sounds to me like you’re just repeating a substantial subset of expressive capability of ConstantExpr, and there isn’t a huge benefit to this approach over just semantically restricting what can appear in an alias.
>>>>
>>>> It is not a substantial part. There is a *lot* more in ConstantExpr.
>>>> As an extreme example, ConstantExpr can trap.
>>>
>>> 1. That is clearly a mistake.
>>>
>>> 2. You are not going to be able to fix that mistake with a change in
>>> representation.  In general, the operands of a trapping operation are
>>> otherwise legal constants, so the only way to intrinsically remove the
>>> potential for trapping constants is to remove all operations from
>>> ConstantExpr that could possibly trap on any operand.  I don’t think
>>> you have community support for that, so assuming that that’s the
>>> inevitable direction is not actually reasonable.
>>>
>>> 3. There is a much more obvious solution: we should just document
>>> that ConstantExprs which are guaranteed to trap are illegal, and then
>>> check that in the verifier.  Constant propagation should already be
>>> doing this analysis to decide that it can safely turn an instruction
>>> into a constant.
>>
>> Concisely, this saves almost no work over removing ConstantExpr entirely but falls quite a bit short of being a comprehensive fix. You have all the difficulty of dealing with autoupgrading code that uses ConstantExpr udiv&global1,&global2, but without going all the way to having a sane new design.
>
> 1. I don’t think we need to be able to autoupgrade code that we declare is ill-formed.
> 2. Your design is not sane.
>
>>>>> It also seems to me like the set of expressions supported by global aliases is only likely to increase.  It’s a pretty natural way to express absolute symbols, for example.
>>>>>
>>>>> Note that there are already practical restrictions on the kinds of ConstantExpr that can appear in various places — e.g. you can make a ConstantExpr that divides the address of a global variable by 7, but there aren’t any relocations that support that, so…
>>>>
>>>> So it is better to have a more restrictive representation.
>>>
>>> I don’t see why.  Your more restrictive representation is just going to
>>> arbitrarily limit the ability of frontends and backends to achieve things
>>> that your representation doesn’t realize are possible.  LLVM IR should
>>> not be in the business of knowing what common object file formats
>>> are capable of expressing.
>>
>> On the contrary, that is precisely the plan (my plan). Why shouldn't it?
>>
>> Suppose we come up with a new set of constants that represent the set of relocations available in the targets we support. This would permit us to generate optimal code for those targets, instead of our current situation of producing worse code on Linux/ELF because Mach-O doesn't support certain kinds of relocation.
>
> Your proposal is seriously to introduce relocations as a first-class concept in LLVM IR?

Yes, filling the role ConstantExpr fills now.

   What possible benefit does that have over just allowing them to be 
expressed naturally as ConstantExprs and then ensuring that front-ends 
and transforms are smart about not introducing ConstantExprs that the 
tool chain / target can’t handle?  You know, like they already do today, 
both for relocations and for a thousand other features that might be 
impossible on the target.

I need to pull this apart a bit to answer it. Why get rid of ConstantExpr?

ConstantExpr has the wrong design. The constants available in LLVM are 
typed bags of bits, the address of a function, the address of a global 
variable, an alias, and relocations. ConstantExpr is modeled as a mirror 
of the set of LLVM instructions -- and which of those things in the 
previous list correspond to instructions? Immediately we see problems 
with the model, there are instructions aren't mirrored -- nobody has a 
call ConstantExpr, or a load, or unreachable. Trapping is just another 
problem on the list.

ConstantExpr's also try to offer other guarantees, like guarantees 
they've been canonicalized. We can do this because unlike instructions 
there's a centralized getter. The complexity of constantexpr 
canonicalization gets worse as the matching instructions change. When 
should 'add x, y' and 'add nsw x, y' be canonicalized to the same 
constantexpr, when 'x' and 'y' aren't concrete yet? There are places in 
LLVM which rely on the former fact that certain subsets of CEs will get 
canonicalized, and which now miscompile because instructions got more 
complex. For a personal pet peeve, GlobalOpt lost its war when 
getelementptr grew the ability to have arbitrary precision integers for 
its indices -- "gep x, i5 0" and "gep x, i6 0" as two different constant 
expressions. (And how would we canonicalize those? When we don't have 
DataLayout and thus don't know the pointer width? To i1 0 because that's 
the smallest size that '0' fits in?) GlobalOpt used to work, an 
implementation of symbolic execution that relies on ConstantExpr 
(checked for simplicity) as keys to simulated memory. We don't see the 
miscompiles with clang because these go away thanks to a second constant 
folder that requires data layout to be present.

Speaking of which, how do we canonicalize ConstantExpr's? The creation 
methods minimize their analysis by assuming canonical inputs, since you 
can't possibly have gotten anything else. What happens when we have two 
constant folders? Or better, what does it mean to take a Module that was 
built without any DataLayout and then add a DataLayout to it? It turns 
out that the answer is patchwork between people who think we need to 
revisit all interior ConstantExpr's inside a ConstantExpr and people who 
think we don't. Some places we do it, in others we don't. Which answer 
you think is correct depends on how much weight you place on certain use 
cases of llvm as a library vs other uses of llvm.

Now, how do we make sure we don't build invalid ConstantExpr's? Well 
currently GlobalOpt just refuses to build anything too complicated for 
the intersection of our supported platforms. As I alluded to in my last 
email, there are cases globalopt drops on the floor that GCC (on Linux) 
optimizes just fine, because globalopt doesn't know that ELF can do 
things Mach-O can't. If we want to get these right, then we need to make 
our constant expr creation depend on the target. So that's step one: 
make target information a mandatory part of the Module, instead of optional.

I haven't settled on an API design (again, I wasn't yet ready to bring 
the proposal to llvmdev) but the rough idea would be something that you 
point at an instruction and it rolls it up into a new-form-constantexpr 
if possible on the target at hand, or returns null if they need to stay 
as instructions. This isn't an impressive API, it requires creating 
instructions even if you know you don't need them, it doesn't help you 
find the best covering set of constants for a given body of 
instructions, but it serves as a starting point and proof of feasibility.

> It’s not as if using magic named relocations is going to actually guarantee anything, since different classes of “relocations” are legal in different situations and given different sets of operands, and since tool chains do evolve and improve, as do dynamic linkers.

What value is there in supporting arbitrarily complex or long constant 
expressions?

Either way, we need to teach the compiler about a new relocation before 
we can use it. You aren't gaining anything by deciding to lower CEs to 
relocations in the backend. Even if your plan is to keep CEs in the IR 
then feed that IR to a newer llvm whose backend knows about the new 
relocations, the only difference is that the newer llvm would have to 
try refolding relocations when loading older IR.

The guarantee we'll get is that constants in the IR should be treated as 
having zero cost. Right now we treat ConstantExpr's as zero cost in loop 
unrolling and in inlining heuristics, but then lower them to 
instructions in the backend (if we're "lucky" and they show up as an 
instruction operand instead of a globalvariable initializer, in which 
case we report_fatal_error).

Yes I'm aware that not all constants have the same cost. A ConstantInt 
may require multiple instructions to materialize on MIPS, etc.

> The problem you’ve identified is because LLVM IR is being unnecessarily paternalistic.  The solution is not to introduce extra obstacles and be even more paternalistic.

The problem I've identified is that LLVM IR is too high level and is 
providing an abstraction that doesn't match what actually happens at the 
low level. Replacing the IR representation corrects the problem by 
making the IR match reality.

Nick