[PATCH] Implement function prefix data as an IR feature.

[Peter Collingbourne]

On Tue, Jul 30, 2013 at 10:57:30PM -0700, Chris Lattner wrote:
> On Jul 30, 2013, at 7:10 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> > Now that you've brought it up, is it true that Darwin prohibits this
> > kind of thing?  After Duncan brought it up I wrote a small test program
> > (load.cpp) which I compiled (loadcpp.orig.s) and modified (loadcpp.s).
> > The resulting program worked as I expected.  Maybe it only fails to
> > work in cases which I haven't considered.
> 
> Yes, it is not valid - usually.  Darwin uses a directive (.subsection_via_symbols iirc) that tells the linker it can "atomize" the object file, splitting it at public labels.  This directive is technically optional, but not using it causes serious pessimization to the toolchain. Ask the lld folks about it.  It is best to assume that Darwin requires chunks of the object file to start with a label.

OK.

> >> - What is the behavior of inlining and other IPO w.r.t. this?  If this requires disabling all inlining and other IPO, then we need to know how to model it and I'd like to know how frontends are going to use it.
> > 
> > I designed this feature so that a call to a function with prefix data
> > has the same semantics as a call to a function without, so IPO passes
> > shouldn't be affected.
> 
> Ok, but what if the serialized metadata is encoding something about an argument, and DAE removes the argument in question?  Is it safe for the IPO optimizer to assume anything about these sorts of functions?  Maybe they should be considered to be of the same class as llvm.used?

I would think that if DAE finds it valid to remove an argument from
a function then the function's address cannot escape the module.
Having read the implementation it seems that DAE is inhibited if
the function has non-local linkage or the module does anything with
the function reference other than call it directly.  In these cases
it shouldn't matter what is stored in the prefix data (and it can
probably be discarded) as DAE has visibility of all callers (and
vacuously all accessors).

I can imagine that a language implementation might need to do something
like unwind the stack in order to read metadata from functions.
But only the implementation will know whether it needs to do this
(UBSan doesn't, for example) and it might need to do other things
as well (such as disabling inlining) so I think the mechanism for
inhibiting things which might affect prefix data should be orthogonal
to whether the data is actually present.

> >  The prefix data only has semantics when the
> > function pointer is bitcasted and loaded from.  This is because of
> > the requirement on the prefix data that it must transfer control to
> > the point immediately succeeding the prefix data, without performing
> > any other visible action.  (This property enables a number of other
> > optimisations; e.g. we can drop prefix data from functions with
> > internal linkage when the prefix data is provably never accessed,
> > or demote an internal linkage function with prefix data to a global
> > constant if the prefix data is read but the function is provably
> > never called.)
> 
> Ok, but what does it mean if inlining happens?  Does the presence of this data block inlining?  Should it?

If inlining happens then the only thing that would be different would
be that the prefix data for the callee would disappear (as far as
the caller's call site is concerned -- if the callee still needs to
be emitted on its own for some other reason it will be present at the
callee's own entry point).  For some clients this might be significant
(see above) but for others it won't and hence there would be no change
in semantics.

I don't think presence of the data alone should block inlining for
the reasons I discussed above.

> >> - In Function.h, this is really unfortunate:
> >> 
> >> +  bool hasPrefixData() const {
> >> +    return getNumOperands() != 0;
> >> +  }
> >> +
> >> +  Constant *getPrefixData() const {
> >> +    assert(hasPrefixData());
> >> +    return cast<Constant>(Op<0>());
> >> +  }
> >> +
> >> +  void setPrefixData(Constant *PrefixData);
> >> 
> >> We want to keep the sizeof(Function) as small as possible, and this feature will almost never be used.  I'd much rather see this be stored with a single bit in Function::SubclassOptionalData (from Value) that indicates "has entry in an on-the-side hash table" (and also is used to implement hasPrefixData()) and store the Constant* in the hashtable.
> > 
> > What if someone RAUWs one of the constants?  Wouldn't we need to
> > introduce a new User subclass that would be stored in the hash table
> > and hold the prefix data as an operand?
> 
> We don't generally support RAUW on constants (e.g., turning "i32 4" into "i32 5" doesn't make sense).  For things that do (e.g. those derived from global values) making the value in the hashtable be a WeakVH should work fine.

It seems that the bigger concern is not RAUW but deletion predicated
on use lists.  I'm not sure if value handles (especially WeakVH) are
appropriate because the prefix data may contain a reference to a global
which needs to be kept around (see test/Feature/prefixdata.ll for an
example of this).  That test does in fact assert if I modify the code
to use value handles (see attached patch, applies on top of the patch
I already sent out), because of this code in GlobalOpt::ProcessGlobal:

  if (GV->use_empty()) {
    DEBUG(dbgs() << "GLOBAL DEAD: " << *GV);
    GV->eraseFromParent();
    ++NumDeleted;
    return true;
  }

While we could in principle patch anything that touches dead constants
to also check Value::hasValueHandle(), this would probably be a lot
of work to get right (there are a lot of uses of use_empty in the
codebase) and it would probably pessimise cases where the module
uses metadata, as I understand that metadata is implemented using
value handles.

> >> - Should this be modeled as some kind of crazy attribute?  That gives you a *different* place to unique it into and also would avoid punishing clients that aren't using it.
> > 
> > I initially wanted to implement this as an attribute, but I realised
> > that we'd have the same RAUW problem I mentioned above.  It may well be
> > better to do the work to allow attributes to hold Values rather than
> > implementing something for prefix data that can't be reused.  I also
> > don't know how difficult maintaining the bit in SubclassOptionalData
> > would be (or if it's worth maintaining it) if we store this as an
> > attribute.  I'd like to get Bill's views on this as well.
> 
> I don't think the RAUW problem is a serious one.  BTW, if you'd like to talk about this, I'll be at the social on Thursday.

I'll see if I can find some people here to talk to about this.
Otherwise, let's talk then.

Thanks,
-- 
Peter