[PATCH] D75590: [Attributor] IPO across definition boundary of a function marked alwaysinline

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D75590#1906851 <https://reviews.llvm.org/D75590#1906851>, @bbn wrote:

> I have fixed the problem that `FnScope` may be null and cached the result of isInlineViable. I have also updated my test file to check the attributes.


When I said "cache the result" I meant that we should ensure to call `isInlineViable` for each function at most once. Maybe at the beginning of the `Attributor::run` method we can go over all the functions and categorize them. If they have exact linkage *or* are `alwaysinline` and and viable to be inlined, we say they are "IPO amendable" and we put them in a set. We can then lookup in the set if the function is fine w/o the need to look for all criteria again. Does that make sense?



================
Comment at: llvm/test/Transforms/Attributor/alwaysinline.ll:9
+; so this will be inlined, although the function is not exactly
+; defined.
+
----------------
What do you mean by "removed safely" here?


================
Comment at: llvm/test/Transforms/Attributor/alwaysinline.ll:21
+
+; CHECK: Function Attrs: nofree nosync nounwind
+define void @outer1() {
----------------
is this one not readnone?


================
Comment at: llvm/test/Transforms/Attributor/alwaysinline.ll:33
+; The function is not alwaysinline and is external
+; so it will be analyzed
+; CHECK: Function Attrs: nofree nosync nounwind readnone willreturn
----------------
We analyze and annotate it because it is an exact definition, see `Function::hasExactDefinition()`. Make the function `linkonce` (or add another pair with `linkonce`) to see the difference.


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

https://reviews.llvm.org/D75590