Page MenuHomePhabricator

[clang][NewPM] Fixing remaining -O0 tests that are broken under new PM
ClosedPublic

Authored by leonardchan on May 21 2019, 4:42 PM.

Details

Summary

This is a patch that should go on top of D58375 which addresses/fixes tests that fail under new PM with -O0.

Current reasons for fixes (from other child patches attached to this one):

  • Pass false to the new PM AlwaysInliner to prevent emission of lifetime start/end intrinsics.
  • Add -fno-experimental-new-pass-manager to tests that use -O (but did not specify a number)
  • Add an AssumptionCache callback to the InlineFuntionInfo used for the AlwaysInlinerPass to match codegen of the AlwaysInlinerLegacyPass to generate llvm.assume for -O0 tests.
  • Some tests explicitly test for cases between both PMs but not all of the legacy PM ones specify -fno-experimental-new-pass-manager.
  • Some New PM debug pass manager output is different from that of the legacy PM. Added a separate run for tests which check that certain passes are run for PGO.
  • Add a check that the dwarf fission mode is split when generating a split dwarf file.
  • Some of the avx-builtin tests emit extra bitcasts which seem to add unnecessary bitcasts. Logically, the behavior is still the same with these bitcasts, but we just check for them if running with the new PM. I couldn’t come up with a better way to ignore/remove them.
  • Some of the stores and allocas in CodeGen/avx512-reduceMinMaxIntrin.c were out of order under new PM, but the behavior of the function is still the same. For this, we just ignore pointers that are meant to be loaded/stored in specific lines. I'm not sure if this is the best solution for this tho.

Fixes in this patch:

  • CodeGen/flatten.c will fail under new PM becausec the new PM AlwaysInliner seems to intentionally inline functions but not call sites marked with alwaysinline (D23299)
  • Tests that check remarks happen to check them for the inliner which is not turned on at O0. These tests just check that remarks work, but we can make separate tests for the new PM with -O1 so we can turn on the inliner and check the remarks with minimal changes.

Diff Detail

Repository
rL LLVM

Event Timeline

leonardchan created this revision.May 21 2019, 4:42 PM
echristo added inline comments.May 21 2019, 5:16 PM
clang/lib/CodeGen/BackendUtil.cpp
1104–1105 ↗(On Diff #200606)

Can you elaborate more here? We do turn on the always inliner at O0 which makes this comment a bit confusing.

clang/test/CodeGen/aarch64-neon-fma.c
235 ↗(On Diff #200606)

Do we really need to check for it or does the autogeneration of testcases do some of this?

clang/lib/CodeGen/BackendUtil.cpp
1104–1105 ↗(On Diff #200606)

I guess he means

We always pass false here since according to the legacy PM logic for enabling lifetime intrinsics, they are not required with O0
leonardchan marked 4 inline comments as done.May 22 2019, 9:15 AM
leonardchan added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
1104–1105 ↗(On Diff #200606)

Yup, my bad. This is what I meant with this comment. Always inlining is used. It's the lifetime intrinsics that aren't always used.

clang/test/CodeGen/aarch64-neon-fma.c
235 ↗(On Diff #200606)

I do not know if this is actually an autogenerated test, but if so then probably not. Is there a way to check this?

leonardchan marked an inline comment as done.
leonardchan edited the summary of this revision. (Show Details)
leonardchan edited the summary of this revision. (Show Details)
leonardchan edited the summary of this revision. (Show Details)

OK, this should be all the tests and now requesting reviews. Most of the tests just require checking different output that is slightly different between pass managers whereas others were functional changes that involved adding some passes to the new PM pipeline or changing the AlwaysInliner somehow.

Are the behavior differences between the newpm alwaysinliner and the oldpm alwaysinliner intentional? Specifically, the differences in pass remarks, and the differences in the treatment of the alwaysinline attribute seem suspect. (I'm not that interested in the different bitcasts and different order of inline allocas, although that might be nice to fix if it's a small change.)

Are the behavior differences between the newpm alwaysinliner and the oldpm alwaysinliner intentional? Specifically, the differences in pass remarks, and the differences in the treatment of the alwaysinline attribute seem suspect. (I'm not that interested in the different bitcasts and different order of inline allocas, although that might be nice to fix if it's a small change.)

This is something for @chandlerc to answer, but I think based off D23299 the different treatment of alwaysinline was intentional and the other aspects of it that this patch addresses like the AddAlignmentAssumptions are probably things that were overlooked at the time.

For now, I made the avx512-reduceMinMaxIntrin.c test only run under the legacy PM since there were already too many changes in this patch. The changes were lit expecting allocas for temporary variables to be in specific orders which weren't respected in the new PM for some reason, but still followed the same logic. I can update this in a later patch separated from this one to make reviewing easier.

chandlerc requested changes to this revision.Jun 10 2019, 5:46 PM

I think this ultimately needs to be split up into smaller patches. A bunch of these things can be landed independently. Here is my first cut at things to split out, each one into its own patch.

  1. the LLVM change to the always inliner
  2. the Clang change to how we build the always inliner
  3. the PGO pipeline changes (which I have to admit I still don't fully understand)
  4. The additions of -fno-experimental-new-pass-manager for test cases that are explicitly testing legacy PM behavior
  5. switching tests to be resilient to changes in attribute group numbering (and adding a RUN line w/ the new PM to ensure we don't regress)

for #5 (or others) where *some* testing needs to be working before they can land, just sequence them after whatever they depend on

Other things I think can also be split out, but I suspect into *different* changes from what you have here:

  1. Instead of passing -fno-experimental-new-pass-manager for tests that use -O but don't specify a number, Clang should pick a consistent value for the level I think

I'd be interested to then see what is left here.

clang/lib/CodeGen/BackendUtil.cpp
1104–1105 ↗(On Diff #200606)

I don't think explaining it in terms of one pass manager or another is the righ thing to do.

Instead, I'd say what the desired result is:

Build a minimal pipeline based on the semantics required by Clang,
which is just that always inlining occurs. Further, disable generating
lifetime intrinsics to avoid enabling further optimizations during
code generation.
This revision now requires changes to proceed.Jun 10 2019, 5:46 PM
leonardchan retitled this revision from [clang][NewPM] Fixing -O0 tests that are broken under new PM to [clang][NewPM] Fixing remaining -O0 tests that are broken under new PM.
leonardchan edited the summary of this revision. (Show Details)

I think this ultimately needs to be split up into smaller patches. A bunch of these things can be landed independently. Here is my first cut at things to split out, each one into its own patch.

Done. I split them into 6 other patches plus this one.

  1. the LLVM change to the always inliner

Addressed in D63170.

  1. the Clang change to how we build the always inliner

Addressed in D63153.

  1. the PGO pipeline changes (which I have to admit I still don't fully understand)

Addressed in D63155. There aren't any logical changes to PGO in this. This just adds PGOInstrumentationGenCreateVar to the pipeline such that ./default.profraw is generated and allows Profile/gcc-flag-compatibility.c to pass.

  1. The additions of -fno-experimental-new-pass-manager for test cases that are explicitly testing legacy PM behavior

Addressed in D63156. I also combined #6 in this one since those -O tests are pretty much -O2 tests.

  1. switching tests to be resilient to changes in attribute group numbering (and adding a RUN line w/ the new PM to ensure we don't regress)

    for #5 (or others) where *some* testing needs to be working before they can land, just sequence them after whatever they depend on

Addressed in D63174. The tests that were failing due to attribute numbering don't seem to fail anymore, so someone must've addressed that in a previous patch. This one now just contains the tests that produces slightly different IR.

Other things I think can also be split out, but I suspect into *different* changes from what you have here:

  1. Instead of passing -fno-experimental-new-pass-manager for tests that use -O but don't specify a number, Clang should pick a consistent value for the level I think

D63168 seems separate from the rest and fixes CodeGen/split-debug-single-file.c.

I'd be interested to then see what is left here.

The remaining ones here are the flatten tests and optimization remark tests. The flatten tests fail since the new PM AlwaysInliner seems to inline functions only and not calls (based off the description in D23299), so for now they are marked as UNSUPPORTED.

The optimization remark tests fail since it seems that the new PM inliner is not enabled at -O0 and emit slightly different remarks. This patch forces those breaking runs to run with legacy PM only and adds separate new PM tests with -O1 to enable the inliner.

chandlerc accepted this revision.Jun 18 2019, 7:41 PM

FWIW, this LGTM. We may want to tweak the behavior around flattening, but that can happen as a follow-up, and this seems to get us to a better state.

This revision is now accepted and ready to land.Jun 18 2019, 7:41 PM
This revision was automatically updated to reflect the committed changes.