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

[Andrew Trick]

Hi Dan. Nice explanation. I have some questions below, but mostly evangelism.

On Dec 5, 2012, at 5:03 PM, Dan Gohman <dan433584 at gmail.com> wrote:
> 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. 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);

That ties the meta-intrinsic to its uses, and obfuscates the def-use
chains. I would not like working with that representation. In general,
I see no reason to move toward program dependence graphs or
SSI. We can continue the LLVM style of making analyses work with
SSA+CFG (with dominance). I happen to like that style.

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

Why are we avoiding optimizations that require dominance? Is there any
problem with allowing some set of instcombines to use dominance if
it's available? This seems crazy not to do in a pass that preserves
the CFG.

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

We need the concept of a meta-intrinsic that doesn't interfere with DCE
or code motion. If the intrinsic expresses an invariant, it would apply to
anything that it dominates, so it implicitly assigns control dependence
to metadata. The meta-intrinsic approach is being used for debug info
and could be generalized to work well for a number of things that are
handled poorly today:

It makes more sense than the front-end's "unreachable" trick which has
pass ordering problems and is awkward, innefficient, and non-scalable.

It would have been a proper way to implement the "expect" instrinsic,
which currently needs to be stripped before any other pass runs.

It would handle load range data without special casing it for integer
load instructions. It would allow the ranges to depend on a condition.

It could provide a framework for supporting lifetime and invariant
markers. We have too many special cases here, and I doubt
that our optimizations are currently robust in their presence.

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

Well said. I appreciate that this is a problem worth solving, but I think
adding ephemeral values to the IR will cause a perpetual headache. We
need to consider other solutions.

I would like the underlying object to have metadata that captures its
alignment, and possibly offset relative to an aligned address. I don't
see why this couldn't be done with an intrinsic that takes a metadata
argument refering to the underlying object's value.

-Andy

> 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.
> 
> 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.
> 
> 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
>> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits