[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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101011