[llvm-commits] [PATCH] Invariants (and Assume Aligned) - LLVM
Hal Finkel
hfinkel at anl.gov
Wed Dec 5 20:15:58 PST 2012
----- Original Message -----
> From: "Dan Gohman" <dan433584 at gmail.com>
> To: reviews+D150+public+94bd4758cb777256 at llvm-reviews.chandlerc.com
> Cc: hfinkel at anl.gov, llvm-commits at cs.uiuc.edu
> Sent: Wednesday, December 5, 2012 7:03:24 PM
> Subject: Re: [llvm-commits] [PATCH] Invariants (and Assume Aligned) - LLVM
>
> This patch assumes that asserted conditions are globally true. This
> significantly restricts their usefulness. Everything's easy if we
> just
> think about asserting properties for an entire function:
>
> void foo(void *p) {
> __builtin_assume(p is aligned or whatever);
> use(p);
> }
>
> However, as soon as a function like this can get inlined into a
> perfectly reasonable callsite like this:
>
> if (p is aligned or whatever)
> foo(p);
> else
> do_something_slow(p);
>
> it doesn't work anymore, because it would apply to all uses of p in
> the caller, including uses where p isn't aligned.
>
> It seems that the intuitive semantics would be that an assertion
> should hold for all the uses dominated by an assertion statement.
> Requiring dominator trees for anything that wants to use assertion
> information would unfortunately keep it out of instcombine, where it
> would be very useful, so this is a significant problem.
Good point.
> The
> tentatively established way to solve this kind of problem is to make
> the assertion return a copy of the value it is asserting a predicate
> about. Something like this:
>
> pp = __buiiltin_assume(p, p != 0);
>
> Then, the predicate would be asserted for pp, but not p, and it would
> be anchored in the SSA graph, so the dominance properties would be
> preserved implicitly, rather than requiring explicit dominance
> queries. The presence of this intrinsic in the SSA graph will require
> special-casing in passes like instcombine which do pattern-matching,
> to avoid missing patterns.
Indeed, this is very similar to how gcc handles the __builtin_assume_aligned intrinsic. The alignment assumption holds only for the returned value. This could certainly work, although I'm not sure how we would handle conditions involving multiple variables.
>
> Note that this approach doesn't automatically eliminate the problem
> of
> anchoring the intrinsic with respect to the surrounding control flow
> and side effects. Presumably, it would still be useful to let the
> intrinsic appear to have side effects to achieve this, as it does in
> the current patch. That will interfere with GVN, DSE, LICM, and many
> other optimizations, but these problems can be special-cased around.
The objections to my first patch, which was implemented along these lines, was that there were too many special cases (even in that patch, and I had only touched some of them). In some sense, doing this implements a new kind of bitcast (albeit one with which it is less convenient to work). On the other hand, perhaps my original patch simply had a lot of special cases without a sufficient upside (being that it applied only to the alignment case). With invariants, the cost/benefit analysis might be different.
To propose an alternative, we could consider the invariant to be solely a frontend matter, and lower as:
(things before)
__builtin_assume(x);
(things after)
as
(things before)
if (x) {
(things after)
} else
unreachable;
This, of course, may have its own issues ;)
>
>
> The other big topic to discuss in this patch is ephemeral values.
> This
> would be a significant landmark change.
>
> Ephemeral values add uses of values to the program which wouldn't
> otherwise be there. This has its purpose of course, but also its
> downsides, as it will interfere with use_empty() and hasOneUse()
> optimizations (much of instcombine, etc.). It could easily be a
> dickens for indvars and lsr, unless, again, it gets the right
> special-case handling. It'll need special-casing in several analyses
> which walk def-use lists. The new EphemeralValues analysis pass will
> make the special-casing less painful, but it's still a value map that
> has to be passed around and kept current.
This does seem to be the major downside (aside from the scoping issues).
>
> There are also optimizations that ephemeral values will break that
> won't be easily special-casable. For example:
>
> *r = something();
> p = *q;
> __builtin_assume(p);
> *r = 0;
>
> If we don't know whether q and r alias, the presence of the
> intrinsic,
> and thus the ephemeral load, is preventing the first store from being
> eliminated. Ephemeral values can also hold branches ephemerally live,
I think we would need special handing of this, but it could be dealt with.
> so they can hold early loop exits ephemerally live, which means they
> could complicate loop optimization. It's a design principle in LLVM
> that the main middle-end optimizations work to enable other
> optimizations. Introducing artificial constructs which hold values
> and
> branches and stores live interferes with this in a very philosophical
> way.
You're right; and having the invariants could certainly interfere with optimization. One way of looking at it is this: Representing the invariant using IR will naturally keep the invariant itself from being optimized away. This is a good thing when the invariant is assisting with "large" optimizations: higher-level loop transformations, autovectorization, conditional-branch short-circuiting, etc. This is probably not a good thing after those kinds of transformations have run and we're more concerned with lower-level transformations. A good trade-off may be that at some point, unless we're doing LTO, we should drop the invariants.
>
> How big of a practical problem is all this special casing and/or
> hampered optimization? If this mechanism is being aimed at only a few
> targeted purposes, probably the gains will outweigh the costs. But,
> if
> invariants become widely used (for example, if they're used to
> describe the special properties of C++ reference types), they could
> be
> come significantly more troublesome and it's less clear overall.
Agreed.
>
> On the code itself:
>
> I assume that asserting false incurs immediate undefined behavior,
> right? If so, then asserting undef may do the same, so the code in
> lib/Transforms/Utils/Local.cpp is not as aggressive as it could be.
> Is
> this intentional? If so, a comment on the philosophy would be
> helpful.
> Also, it's not clear why undef is important to special-case there at
> all.
Thinking about it, perhaps the true value being provable false should be made equivalent to an unreachable.
Thanks again,
Hal
>
> Dan
>
> On Wed, Dec 5, 2012 at 12:43 PM, hfinkel at anl.gov <hfinkel at anl.gov>
> wrote:
> > Changed to using @llvm.invariant; added analysis for ephemeral
> > values.
> >
> > http://llvm-reviews.chandlerc.com/D150
> >
> > CHANGE SINCE LAST DIFF
> > http://llvm-reviews.chandlerc.com/D150?vs=384&id=439#toc
> >
> > Files:
> > docs/LangRef.html
> > include/llvm-c/Transforms/Scalar.h
> > include/llvm/Analysis/EphemeralValues.h
> > include/llvm/Analysis/InlineCost.h
> > include/llvm/Analysis/Passes.h
> > include/llvm/InitializePasses.h
> > include/llvm/Intrinsics.td
> > include/llvm/LinkAllPasses.h
> > include/llvm/Transforms/Scalar.h
> > lib/Analysis/Analysis.cpp
> > lib/Analysis/CMakeLists.txt
> > lib/Analysis/CodeMetrics.cpp
> > lib/Analysis/EphemeralValues.cpp
> > lib/Analysis/InlineCost.cpp
> > lib/Analysis/ValueTracking.cpp
> > lib/CodeGen/IntrinsicLowering.cpp
> > lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
> > lib/Transforms/IPO/InlineSimple.cpp
> > lib/Transforms/IPO/PassManagerBuilder.cpp
> > lib/Transforms/Scalar/ADCE.cpp
> > lib/Transforms/Scalar/AlignmentInvProp.cpp
> > lib/Transforms/Scalar/CMakeLists.txt
> > lib/Transforms/Scalar/Scalar.cpp
> > lib/Transforms/Utils/Local.cpp
> > test/Analysis/EphemeralValues/lit.local.cfg
> > test/Analysis/EphemeralValues/simple.ll
> > test/Transforms/AlignmentInvProp/lit.local.cfg
> > test/Transforms/AlignmentInvProp/simple.ll
> > test/Transforms/Inline/ephemeral.ll
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
--
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list