[PATCH] D23299: [PM] Port the always inliner to the new pass manager in a much more minimal and boring form than the old pass manager's version.

[Chandler Carruth via llvm-commits]

chandlerc marked 2 inline comments as done.
chandlerc added a comment.

Thanks for the review. See some responses inline, but largely good catches! Updated patch momentarily.


================
Comment at: lib/Passes/PassRegistry.def:40
@@ -39,2 +39,3 @@
 #endif
+MODULE_PASS("always-inliner", AlwaysInlinerPass())
 MODULE_PASS("constmerge", ConstantMergePass())
----------------
davide wrote:
> Is there a particular reason why you renamed `always-inline` to `always-inliner`?
> I think we should try to keep the names of the passes consistent between old and new PM (unless there's a reason not to)/
No actually... I'm not sure what the best thing to do in this particular case is...

The class is named AlwaysInliner and that seems the right thing -- it should be a noun, etc. I've adjusted the file names to match which seems the right thing.

I had adjusted the pass name to match as well, but I agree it is a bit vague whether we should do this or not. The pass names don't actually have the noun pattern necessarily. For now, I'll take this back to the old name. We can always pursue some correspondence between pass name and class name later if that's desirable. Currently (as this diff hunk illustrates) we're no where near anyways.

================
Comment at: lib/Transforms/IPO/AlwaysInliner.cpp:53
@@ +52,3 @@
+        // FIXME: We really shouldn't be able to fail to inline at this point!
+        Changed |= Inlined;
+      }
----------------
davide wrote:
> `Changed |= InlineFunction(CS, IFI);` , no?
Well, the FIXME was about potentially asserting on this inlined variable. But yea, I guess the variable isn't serving any purpose until then. Nuked.

================
Comment at: lib/Transforms/IPO/AlwaysInliner.cpp:94-97
@@ -65,5 +93,6 @@
                       "Inliner for always_inline functions", false, false)
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
 INITIALIZE_PASS_DEPENDENCY(CallGraphWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(ProfileSummaryInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
+INITIALIZE_PASS_END(AlwaysInlinerLegacyPass, "always-inline",
----------------
davide wrote:
> I'm not entirely sure here, but are all these dependencies actually needed?
For the legacy one, I think so, because of the inliner base class. I'm not changing any of it in this patch at least...

================
Comment at: test/Transforms/Inline/always-inline.ll:4
@@ -3,3 +3,3 @@
 ; Ensure the threshold has no impact on these decisions.
-; RUN: opt < %s -inline-threshold=20000000 -always-inline -S | FileCheck %s
-; RUN: opt < %s -inline-threshold=-20000000 -always-inline -S | FileCheck %s
+; RUN: opt < %s -inline-threshold=20000000 -always-inline -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
+; RUN: opt < %s -inline-threshold=-20000000 -always-inline -S | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-CALL
----------------
davide wrote:
> These tests modification (at least part of them) can be committed separately, probably.
I don't think that really makes sense...

The only changes here are to allow one of the test cases to be omitted with the new pass manager's pass because it doesn't support that case. But none of these changes would be required without that, so it seems best to put them into this patch?


https://reviews.llvm.org/D23299