[PATCH] D101011: [Attr] Add "noipa" function attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/IR/Globals.cpp:352
+    llvm_unreachable("Fully covered switch above!");
+  }
 //===----------------------------------------------------------------------===//
----------------
dblaikie wrote:
> jdoerfert wrote:
> > dblaikie wrote:
> > > jdoerfert wrote:
> > > > dblaikie wrote:
> > > > > jdoerfert wrote:
> > > > > > dblaikie wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > The only thing I'm not 100% convinced by is this part. I can see the appeal, but I can also imagine problems in the future. Not everything looking at the linkage might care about `noipa`. I'd rather introduce a helper that checks `noipa` and linkage, and maybe also the alwaysinline attribute. Basically,
> > > > > > > > ```
> > > > > > > >     /// Determine whether the function \p F is IPO amendable
> > > > > > > >     ///
> > > > > > > >     /// If a function is exactly defined or it has alwaysinline attribute
> > > > > > > >     /// and is viable to be inlined, we say it is IPO amendable
> > > > > > > >     bool isFunctionIPOAmendable(const Function &F) {
> > > > > > > >     ┊ return !F->hasNoIPA() && (F.hasExactDefinition() || F.isAlwaysInlined());
> > > > > > > >     }
> > > > > > > > ```
> > > > > > > > whereas the "always inlined" function does not exist yet.
> > > > > > > Yep - I mean this is the guts of it, so the bit that's interesting/debatable from a design perspective (but glad to hear the rest of it/the mechanical stuff is about right)
> > > > > > > 
> > > > > > > I worry about implementing this as a separate property and having to update every pass/place that queries this sort of thing. That'll mean likely changing every call to hasExactDefinition (admittedly there are fewer calls than I was expecting, which also sort of worries me a bit - about 20, most of them in FunctionAttrs) and adding new test coverage for every use? & all the future uses that might find hasExactDefinition and think that's enough.
> > > > > > > 
> > > > > > > Perhaps we could rename hasExactDefinition to something that would capture this new use case and reduce the chance it'd be used for something unsuitable later?
> > > > > > I think "isFunctionIPOAmendable", or similar, makes it much clearer to the observer what is happening. We can also put more things in there, the always_inline logic, we can check for `naked`, etc.
> > > > > > Since any new name/function requires to go to all callers, I don't think there is much to gain/loose (assuming we don't overload hasExactDefinition).
> > > > > You mean not much to gain/lose regarding whether the function is renamed V a new function is added?
> > > > > 
> > > > > Yeah, it's a bit of a stretch on my side, but I feel like renaming and adding some functionality to the function probably doesn't merit adding testing to al callers - but adding a new function and porting the passes over to it, I'd feel compelled to test each pass. How do you feel about either of those perspectives, or some other perspective on the change and testing of it?
> > > > > 
> > > > > 
> > > > yes, renaming vs new function seems to be little different.
> > > > 
> > > > I favor a new function as there might be unknown uses downstream, but I'm not opposing renaming the existing one so we can make it do something else/more. 
> > > > 
> > > > I would create a new helper, select the passes we know should deal with `noipa` explicitly, move them over to the new helper, and add tests. It is unclear how we could get away with less tests if we do something else, I mean without just not testing if a pass honors `noipa` now.
> > > > 
> > > > Function-attrs (and I can port them to the Attributor) is the top consumer. From reading through the uses of "exact definition" I'd say IPSCCP (via canTrackReturnsInterprocedurally) is another one. @arsenm might want to look at `AMDGPUAnnotateKernelFeatures.cpp`. PruneEH and DeadArgElim need updates and tests, though that should be easy. I have no idea what `ObjCARCAPElim.cpp` does, we probably should ping someone. That should cover everything in-tree, I think.
> > > > 
> > > > That said, I'm sure we have some volunteers to do some of the porting testing so it's not all on you.
> > > > (Looking at @xbolva00 ;) )
> > > Yeah, I worry about keeping the existing one in place due to the risk of it being misused out of momentum/existing familiarity, rendering noipa less accurate.
> > > 
> > > Admittedly partly inspired by the GCC implementation ( https://gcc.gnu.org/legacy-ml/gcc/2016-12/msg00064.html ) that implemented noipa by way of marking such functions as interposable - which I tend to agree with. The idea that we have an existing property that this maps to (though there's reasonable disagreement about how accurately) - now we're adding a new way that that property can be expressed. That doesn't necessitate testing all functionality that depends on the property - only testing that the new expression of the property is working correctly. 
> > > Yeah, I worry about keeping the existing one in place due to the risk of it being misused out of momentum/existing familiarity, rendering noipa less accurate
> > 
> > Fair. Maybe we rename it and introduce a new helper ;)
> > 
> > Renaming so downstream users notice, new helper to encapsulate all the "can we do IPA across this call edge" logic without polutting the "is this derefinable" code path.
> > 
> > I generally would value clear naming/design over the desire to "auto-port" existing users to what might be the right thing. While the latter has short term advantages it often is long term painful, and at some point someone will come along and split the linkage logic and the rest apart, or introduce an argument with default value, rendering all these thoughts mood. [Site note: I imagine we will actually have to find a way to split it to update our internalization optimization in which we want to work around derefinable linkage through a TU internal copy but not around `noipa`.]
> > 
> > That said, if people really think hiding `noipa` behind this function is the way to go, I'm not going to block that.
> Actually, looking at this further - maybe it makes sense to actually move it further /down/ rather than up. Closer to how GCC modeled this:
> 
> one layer down, at `GlobalValue::isInterposable` I found a test for the `getParent() && getParent()->getSemanticInterposition()`, the latter uses module metadata to flag the module as semantically interposable.
> 
> The documentation for "isInterposable" seems more accurate to the semantics I want to implement here:
> ```
> /// Whether the definition of this global may be replaced by something
> /// non-equivalent at link time. For example, if a function has weak linkage
> /// then the code defining it may be replaced by different code.
> ```
> 
> (where's `isExactDefinition` says "Inlining is okay across non-exact linkage types as long as they're not interposable" and `mayBeDerefined` says "Returns true if the definition of this global may be replaced by a differently optimized variant of the same source level function at link time." - while the latter is less precise, it still has that worrying "of the same source level function" - which could suggest that a function that may be derefined could still be inlined)
> 
> In fact, adding this noipa attribute could supersede the SemanticInterposition module metadata entirely - frontends could put noipa on every function (as they do with optnone at -O0 today). Should we call it something different than noipa, then? Should it be called semantic_interposition?
> 
On first thought I can see us adding an attribute to replace that metadata, it depends on how it is supposed to work in an LTO setting at the end of the day. That said, `noipa` should exist on its own.

I also still think we should not intertwine two things that are similar but different: "do not perform ipa/ipo" and "the semantics do not allow you to do ipa/ipo, among other things". 

Long story short, I'll continue to be in favor of a new `canIdoIPO(CallBase/Function)` helper which we employ in our passes, though others should chime in if you prefer a solution in which `noipa` is checked in existing lookup functions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101011/new/

https://reviews.llvm.org/D101011