cc: @fedor.sergeev @philip.pfaffe on this

*ping*

*ping* Is it ok to proceed with only checking the new PM output for these tests? If so I could just edit my previous patch to remove the legacy PM run lines since they already include the bitcasts from the new PM.

In D63638#1574927, @craig.topper wrote:

What if we just only check the output from the new pass manager. I don't think I care about the differences between the two.

LGTM. Good catch

In D63638#1574373, @craig.topper wrote:

There's some inliner running because the intrinsics are implemented as always_inline functions and they are clearly being inlined in -O0. In a previous post, Chandler said the new PM has a special inliner for always_inline in -O0 and the old pass manager just used the normal inliner.

In D63638#1560991, @craig.topper wrote:

In D63638#1560846, @spatel wrote:

I skimmed D63174 but haven't applied either of these patches to test locally, so I may not have the full picture.

IMO, we do not want clang regression tests running -instcombine/-instsimplify. That can cause clang tests to break when an underlying LLVM change is made. Forcing LLVM devs to depend on clang and fix the resulting breakage is backwards and unexpected extra work. This has happened to me several times.

As a compromise to the -O0 IR explosion, we do have precedent for running the optimizer's -mem2reg pass since that doesn't change frequently at this point.

And I haven't tried this, but we do have utils/update_cc_test_checks.py - this is supposed to take the manual labor out of generating assertions in the same way that we do in the optimizer and codegen regression tests with utils/update_test_checks.py and utils/update_llc_test_checks.py. Can you start with that and remove the irrelevant CHECK lines, so only the common/important lines remain? Or just use independent FileCheck '--check-prefixes'?

I definitely agree running -instcombine would be bad since it can replace squences with other sequences. -instsimplify is a little less scary because our intrinsic tests shouldn't really have a lot of things that are trivially reducible. Though that may not be as true as I want it to be. The main issue we seemed to need -instsimplify for with the new pass manager is to merge redundant bitcasts. The inliner in the old pass manager seemed to do that itself, but the new pass manager's always inliner doesn't.

*ping* @chandlerc

ping @chandlerc

Reverted in r364692

Understood. I'll revert this patch.

I mean, I'm happy for the patch to be reverted, but I still really don't understand why this fixes the test to work *exactly* the same as w/ the old pass manager and doesn't break any other tests if it is completely wrong? It seems like there must be something strange with the test coverage if this is so far off of correct without any failures...

I also don't understand what fix you are suggesting instead, but maybe you can show a patch?

In D63155#1563229, @xur wrote:

This patch does not make sense to me.

The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline.

This patch add a pass

MPM.addPass(PGOInstrumentationGenCreateVar(PGOOpt->ProfileFile));

which only gives you the wrong signal of instrumentation is done.

I wrote pass PGOInstrumentationGenCreateVar only for some trick interactions for thinlto under ldd for CSPGO.
Regular FDO should not use it.

The right fix is to enable PGO instrumentation and use in pass builder for O0.

I would like to request to revert this patch.

@t.p.northover Would you feel ok reviewing this?

Any updates on this? I'm thinking that in the meantime maybe we could commit D63174 and work on this while that lands. If so, we could get an upstream new PM buildbot that can catch any new PM regressions.

Hmm is there usually a protocol/etiquette for getting a new built-in submitted to compiler-rt?

Is there not a function for that already in compiler-rt? Or is the problem that it doesn't necessarily exist, e.g. if we're supporting a 64-bit scaled integer type but not a 128-bit integer type?

In D63174#1553096, @craig.topper wrote:

I'm going to try to work on the X86 tests. Can we hold off on committing those?

In D63626#1553054, @chandlerc wrote:

See inline comment, but I think we should just drop the testing of the function attribute bit here rather than adjusting the pipeline.

In D63174#1549601, @chandlerc wrote:

OMG, I'm so sorry, I had no idea that the tests would explode like that... Yeah, I don't think that's useful....

Maybe a better approach is to just explicitly run the code through opt -passes=instsimplify before handing it to FileCheck? That should produce almost identical results to the old PM's inliner?

Feel free to just try this out and report back -- if it isn't looking good, don't invest a bunch of time in it, we should talk to Craig and figure out what the right long-term strategy here is.

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?

OK so I instead wrote a script that generated the tests for me since changing each individual case by hand was painful. All the -cc1 tests now match, although I don't know any ways of simplifying the IR further to help reduce test size. I would definitely love to know if there are any ways to simplify the instructions. Unfortunately for convergent.cl, the codegen still differs slightly around the end of the loop when using -instcombine in addition to instnamer:

In D62088#1537470, @bjope wrote:

Please add test cases for scale=0 and scale=width as I assume those need special handling (UB right now?).
And if scale=0 and scale=width needs special handling, then I guess scale=1 and scale=width-1 are new boundary values so I maybe it would be nice to have tests for those scales as well.

For the avx tests, I don't suppose you know a simple way to generate these tests? They're about 10k lines long and it's taking a while to go through it by hand to replace the current IR checks with codegen for -O1. (Perhaps something along the lines of utils/update_llc_test_checks.py in llvm?)

I did some more digging and found the following differences between PMs for each test, and they seem to all differ and can be fixed for different reasons.

In D63153#1540920, @chandlerc wrote:

Code change LGTM. Can you update at least one of the tests to explicitly run both PMs so that we'll notice if this breaks in some weird way? Feel free to submit with that change.