[llvm-commits] [PATCH] Invariants (and Assume Aligned) - LLVM

Hal Finkel hfinkel at anl.gov
Thu Dec 6 08:48:55 PST 2012


----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Dan Gohman" <dan433584 at gmail.com>
> Cc: reviews+D150+public+94bd4758cb777256 at llvm-reviews.chandlerc.com, llvm-commits at cs.uiuc.edu
> Sent: Wednesday, December 5, 2012 10:15:58 PM
> Subject: Re: [llvm-commits] [PATCH] Invariants (and Assume Aligned) - LLVM
> 
> ----- 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.

Thinking about this, handling the multivariable case could be done in a relatively-straightforward way:
%R = call { type1, type2, ... } @llvm.invariant(i1 %cond, type1 %v1, type2 %v2, ...)
%w1 = extractvalue { type1, type2, ... } %R, 0
%w1 = extractvalue { type1, type2, ... } %R, 1

We could map the C intrinsic in the same way (having it return a struct). What do you think?

Thanks again,
Hal

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