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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/IR/Globals.cpp:352
+    llvm_unreachable("Fully covered switch above!");
+  }
 //===----------------------------------------------------------------------===//
----------------
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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101011