[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:
> > 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101011