Page MenuHomePhabricator

[Test] rewrite inline_nossp.ll
ClosedPublic

Authored by nickdesaulniers on Jun 25 2021, 4:42 PM.

Details

Summary

While adding remark based tests in D104944, I noticed that the tests
that we were passing were passing for the wrong reason. They were
passing because the dynamic allocas were preventing inlining, not the
code I added in D91816.

Rewrite and simplify the test. Add remark based checks to validate we're
preventing inline substitutions for the right reasons.

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Jun 25 2021, 4:42 PM
nickdesaulniers created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 4:42 PM
nickdesaulniers edited the summary of this revision. (Show Details)
  • add missing word to commit message
MaskRay accepted this revision.Jun 26 2021, 10:39 AM
This revision is now accepted and ready to land.Jun 26 2021, 10:39 AM
This revision was automatically updated to reflect the committed changes.
abhinavgaba added inline comments.
llvm/test/Transforms/Inline/inline_nossp.ll
20

Looks like this fails with the legacy pass manager. Could you please add -enable-new-pm=1 if this is intended to be limited to the new pass manager?

diff --git a/llvm/test/Transforms/Inline/inline_nossp.ll b/llvm/test/Transforms/Inline/inline_nossp.ll
index 2a4c8c65f892..54b1ba97be8c 100644
--- a/llvm/test/Transforms/Inline/inline_nossp.ll
+++ b/llvm/test/Transforms/Inline/inline_nossp.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -inline -o - -S %s -pass-remarks-missed=inline 2>&1 | FileCheck --check-prefixes=CHECK,CHECK-INLINE %s
+; RUN: opt -inline -enable-new-pm=1 -o - -S %s -pass-remarks-missed=inline 2>&1 | FileCheck --check-prefixes=CHECK,CHECK-INLINE %s
 ; RUN: opt -passes='cgscc(inline)' %s -S -pass-remarks-missed=inline 2>&1 | FileCheck --check-prefixes=CHECK,CHECK-INLINE %s
 ; RUN: opt -always-inline -o - -S %s | FileCheck %s
 ; RUN: opt -passes=always-inline -o - -S %s | FileCheck %s
llvm/test/Transforms/Inline/inline_nossp.ll
20

Doesn't -inline invoke the OPM? I thought -passes=inline was NPM and -inline was OPM? cc @aeubanks

If I'm mistaken, then I should make this -enable-new-pm=0 for -inline and fix the test.

I'm also curious how much longer we expect to support OPM (though perhaps it's better not to ask)?

aeubanks added inline comments.Jun 29 2021, 12:56 PM
llvm/test/Transforms/Inline/inline_nossp.ll
20

-inline will invoke the new PM if the new PM is enabled by default.

-enable-new-pm=0 -inline will always force the legacy PM

I just sent out an RFC yesterday to llvm-dev about updating tests

llvm/test/Transforms/Inline/inline_nossp.ll
20

Yikes! Then all of the existing tests using the OPM style command line flags probably should have been switched to explicitly use -enable-new-pm=0 BEFORE switching the default to NPM. Now we're potentially missing a lot of OPM test coverage that may be bit rotting.

Let me send a patch fixing up this test.

llvm/test/Transforms/Inline/inline_nossp.ll
20