[llvm] r209007 - Fix most of PR10367.

[John McCall]

On May 17, 2014, at 10:42 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> John McCall wrote:
>> On May 17, 2014, at 9:03 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>>> 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.

…because control flow and side effects aren't constants.  Are there side-effectful relocations you’re concerned about representing?  What is the use of a side effect which occurs in an unreliable order at load time?

> 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.

Then LLVM pervasively mis-uses canonicalization.  I don’t see how any of these problems are solved by target-specific relocations, which (1) can overlap in ways that analyses must be aware of and yet (2) inherently can’t be canonicalized or even interpreted without, not just data layout, but a complete target available.

> 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.

If “target information” means “know the exact semantics of an arbitrary list of target- and toolchain- specific relocations”, not just “what’s the target data layout”, that’s a very significant new requirement.

ConstantExprs can be meaningfully interpreted by target-independent code, even if they can’t always be validly generated by it.  Or really, they can always be generated by target-independent code (assuming it can be proven not to trap), they just can’t necessarily be used in certain places.

>> 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.

1. It’s an optimization.  There’s no “have to” here.
2. That code looks exactly the same whether you’re using ConstantExpr or ConstantRelocation.

Both of our proposals involve target-specific IR emission/transformation using target-specific information to produce a constant that’s only valid on specific targets.  The only difference is that you want to make constant completely opaque to target-independent code, even when it’s something obvious like “take the low 17 bits of this pointer”, because you find the possibility that somebody might accidentally form such a constant — and thus require the backend to reject the code — aesthetically unappealing.  In contrast, I feel that allowing the IR to express things that can’t necessarily be emitted by the backend in every context preserves a lot of very useful flexibility.

If I want to use a new relocation, in either proposal, I have to change my frontend/transform to emit a constant that follows a certain pattern, and I have to change the backend to recognize that pattern and use the relocation.  Your proposal also requires me to change some sort of intrinsics-like table in IR and make sure that a bunch of useful target-independent transforms know how to exploit that table to do basic analyses involving my constant.

>> 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.

We strongly disagree on this design, and that’s fine.  But I do not see why it matters, because you not only haven’t proposed it, but you haven’t even fleshed it out to yourself yet, which is, again, fine as far as your proposal for ConstantExpr goes.  The problem is that you and Rafael have decided to go ahead and use the basic thrust of this less-than-half-baked proposal to completely redesign an existing LLVM feature in a way that intentionally constrains and complicates future extensions.  That’s using trunk as an experimental lab for IR design, and it seems totally inappropriate to me.

John.