[PATCH] D63156: [clang][NewPM] Add -fno-experimental-new-pass-manager to tests

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

>> **CodeGen/pgo-sample.c [No new fix]**: The test checks that `PruneEH` runs, but there doesn’t seem to be a corresponding new PM pass for it. Should there be? If there is one then we can just check for that in the debug PM dump.
> 
> The analog would be `function-attrs` and `function(simplify-cfg)` in some combination. Maybe this should just check that some specific thing is optimized away at `-O2` rather than checking the specific details of pass pipeline construction?

For now I added a check that those 2 extra passes are run in the debug pass dump. It seems that the original test just wanted to see that a particular pass was run, so I left it at that, but I can still add a separate test/run for checking that `function-attrs` and `function(simplify-cfg)` replace `invoke` instructions at `-O2` (which is what I think PruneEH does).

> Yeah, I think the entry/exit pass really wants to come *before* any other pass. That should be possible even in the new PM by using an extension point?

Moved this into D63577 <https://reviews.llvm.org/D63577>.

>> **CodeGenCXX/conditional-temporaries.cpp**: The new pm seems to be unable to statically determine the return value of some functions. For now I just add separate checks for the new PM IR.
> 
> Interesting. Definitely the right thing for this patch. Maybe file a PR to track understanding why the new PM struggles here.
> 
>> **CodeGenCXX/member-function-pointer-calls.cpp**: Same as `CodeGenCXX/conditional-temporaries.cpp`. In this case, the new PM codegen also contains calls to lifetime start/end intrinsics. Perhaps this time it could be due to the intrinsics not being optimized out?
> 
> Weird, but that's also my best guess. Again, maybe we need a bug to track why this is different.

Created https://bugs.llvm.org/show_bug.cgi?id=42333 to track these 2.

>> **CodeGenObjC/os_log.m**: Legacy test expects argument of `ptrtoint` and some functions to be a specific type and argument, though the new PM codegen uses a different type and argument which are also valid since `@llvm.objc.retainAutoreleasedReturnValue(val)` always returns `val`.
>> 
>> **CodeGenObjCXX/os_log.mm**: A function seems to have been inlined under the new PM. Here we just prevent inlining for the new PM run since the test expects this specific function call.
> 
> You could also use a `noinline` attribute in the code to express the *specific* call that needs to be retained?

Hmm `noinline` doesn't seem to be working in this case. The call is made to a code generated builtin function (`__os_log_helper`).



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1135-1139
+      PB.registerOptimizerLastEPCallback(
+          [](FunctionPassManager &FPM, PassBuilder::OptimizationLevel Level) {
+            FPM.addPass(EntryExitInstrumenterPass(/*PostInlining=*/false));
+          });
+
----------------
chandlerc wrote:
> FYI, feel free to split out the entry/exit change (and its test) to a separate patch if you want. Either way really.
Separated this into D63577


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:815-818
+  // running remaining passes on the eliminated functions. These should be
+  // preserved during prelinking for link-time inlining decisions.
+  if (!LTOPreLink)
+    MPM.addPass(EliminateAvailableExternallyPass());
----------------
chandlerc wrote:
> I'd suggest splitting this into a separate patch to LLVM and adding an LLVM-side test to cover it as well.
Separated this into D63580 and added an LLVM test in that one


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63156