Page MenuHomePhabricator

Add options to enable memoryssa for loopsink, expand loopsink testing and fix exposed bug in LICM
ClosedPublic

Authored by jamieschmeiser on Oct 27 2020, 9:53 AM.

Details

Summary

Add options to enable memoryssa for loopsink (defaulting to false,
retaining the original functionality). Expand existing loopsink
testing to also test loopsinking using new pass manager. The option
exposed a bug that was previously fixed for loopsink without
memoryssa. When sinking an instruction into a loop, the source block
may not be part of the loop but still needs to be checked for
pointer invalidation. This is the fix for bugzilla #39695 (PR 54659)
expanded to also work with memoryssa. The bug is only exposed when
all the changes are combined. Just expanding the tests will not show the
bug nor will just enabling memoryssa in loopsink. They have to be done
together to show the bug hence both and the fix are all done in this PR.

Respond to comments after attempted landing: move test for profile
data earlier to avoid unnecessary computations.

Diff Detail

Event Timeline

jamieschmeiser created this revision.Oct 27 2020, 9:53 AM
jamieschmeiser requested review of this revision.Oct 27 2020, 9:53 AM

Note that @sidbav made contributions to these changes.

  • Could you separate the SinkHoistFlag refactoring into an NFC?
  • Could you clang-format and fix lints?
  • LICM changes look good. AFAICT the MSSA changes in LoopSink are accurate, but it may need more testing than the existing unit tests.
  • Why do you want the legacy pass manager to use the AST and the new pass manager MSSA?

As requested, I have split the refactoring of the SinkAndHoistLICMFlags
into a separate PR and made this PR a child of that.

In answer to the review question, the intention is not to make the 2
passes different, but rather to advance the new pass manager version
to use memoryssa. It has been observed that LoopSinking in a new
pass manager pipeline can take significant amounts of compile time
for some cases which is mitigated by using memoryssa.

I guess my question was more regarding consistency. Why not use MSSA in the LPM as well and drop the AST usage entirely? Or provide a flag that can be used to switch between MSSA and AST version (see LICM or DSE for similar flags using MSSA or alternative analyses), just in case there are users of the AST version.

Respond to review comments. Enable Memory SSA in legacy Loop Sink pass
under EnableMSSALoopDependency option control. Update tests accordingly.

The current behavior is:

  • for the NPM, always use MSSA for LoopSink, which is a Function pass.
  • for the LPM, LoopSink is a Loop pass and it will use MSSA conditioned on the EnableMSSALoopDependency flag which is set to true, hence this patch is switching loop sink to use MSSA.

I would suggest adding a different cl::opt, specific to LoopSink and set it to false in this patch, hence not changing the current behavior. Use this flag for both pass managers (or create two flags if you intend to flip just one).
Then in a subsequent patch change the flag(s) to true and update the pipeline tests.
This will provide a simple revert of the cl::opt value if things go wrong, and an option for potential users to disable the MSSA usage only for LoopSink, not for all the other passes (controlled by EnableMSSALoopDependency).

llvm/lib/Transforms/Scalar/LoopSink.cpp
398

/*CurAST=*/nullptr

443

Only create AST in the absence of MSSA.
The checks in canSinkOrHoistInst assume only one of the two analyses is passed in. For LICM, the asserts for this are in sinkRegion/hoistRegion. I added a similar assert in f514b32a899 to ensure this is true here as well.

jamieschmeiser retitled this revision from enable memoryssa for loopsink in new passmanager, expand loopsink testing and fix exposed bug in LICM to Add options to enable memoryssa for loopsink, expand loopsink testing and fix exposed bug in LICM.
jamieschmeiser edited the summary of this revision. (Show Details)

Respond to review comments by adding options controlling whether
memoryssa is used for loopsink, defaulting to false. Expanded the
tests to include these options.

Note that the tests for passes are still changed because the dependency
on memory ssa is required in order to allow the option of using
memory ssa in the legacy pass manager. If my understanding of this
is incorrect, please let me know and I will remove the dependency
and the changes to the passes tests.

Thank you, just a couple of small comments below.

llvm/lib/Transforms/Scalar/LoopSink.cpp
405–413
if (MSSA)
  PA.preserve<MemorySSAAnalysis>();
464

Use EnableMSSAInLegacyLoopSink here. This should change the tests.

nhaehnle removed a subscriber: nhaehnle.Wed, Nov 4, 8:52 AM

Respond to review comments: properly indicate preserved analyses,
which also means the changes are not needed for the pipeline tests.

asbirlea accepted this revision.Wed, Nov 4, 11:39 AM

lgtm.
Looking forward to having it enabled in a future patch.

llvm/lib/Transforms/Scalar/LoopSink.cpp
258

Beyond the scope of this patch: I'm wondering if there were issues here since the AST was not updated. In LICM, the update looks something like:

if (CurAST)
  CurAST->copyValue(&I, C);
I.replaceAllUsesWith(C);
This revision is now accepted and ready to land.Wed, Nov 4, 11:39 AM
jamieschmeiser reopened this revision.Wed, Nov 18, 12:20 PM

test failures when pushing

The Buildbot has detected a failed build on builder clang-cmake-armv7-quick while building llvm.
Full details are available at:

https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_-23builders_107_builds_1659&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=v5KeUvUQ3NmVKfDF_PHQ197pdpgdKEZ2Cm6JUebPiEs&s=QQkrhJP0JJLKRQfOOzGYX0pkct6JJhzL0CHRaZMbkEE&e=

Buildbot URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=v5KeUvUQ3NmVKfDF_PHQ197pdpgdKEZ2Cm6JUebPiEs&s=_gM68RF9ovfGi9Cd5Z7GlMzchqp751e2xZaWYOIO8Ts&e=
Worker for this Build: linaro-armv7-quick
Build Reason: <unknown>
Blamelist: Jamie Schmeiser <schmeise@ca.ibm.com>
BUILD FAILED: failed 49489 expected passes 19381 unsupported tests 75 expected failures 1 unexpected failures Unexpected test result output 14 (failure)
Sincerely,
LLVM Buildbot

This revision is now accepted and ready to land.Wed, Nov 18, 12:20 PM

The Buildbot has detected a failed build on builder clang-cmake-armv7-quick while building llvm.
Full details are available at:

https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_-23builders_107_builds_1659&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=v5KeUvUQ3NmVKfDF_PHQ197pdpgdKEZ2Cm6JUebPiEs&s=QQkrhJP0JJLKRQfOOzGYX0pkct6JJhzL0CHRaZMbkEE&e=

Buildbot URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=v5KeUvUQ3NmVKfDF_PHQ197pdpgdKEZ2Cm6JUebPiEs&s=_gM68RF9ovfGi9Cd5Z7GlMzchqp751e2xZaWYOIO8Ts&e=
Worker for this Build: linaro-armv7-quick
Build Reason: <unknown>
Blamelist: Jamie Schmeiser <schmeise@ca.ibm.com>
BUILD FAILED: failed 49489 expected passes 19381 unsupported tests 75 expected failures 1 unexpected failures Unexpected test result output 14 (failure)
Sincerely,
LLVM Buildbot

Looks like a flaky testcase, it passed on next run with my changes in on one build and the other looks like a network issue.

This change showed up as a significant compile-time regression (before it was reverted): https://llvm-compile-time-tracker.com/compare.php?from=85ccdcaa502ee2c478f2d0ba2b1e217117b69032&to=d4ba28bddc89a14885218b9eaa4fbf6654c2a5bd&stat=instructions

From what I understood, the usage of MemorySSA in LoopSink is still supposed to be behind a flag, so this seems unexpected to me?

Looking over the patch, the compile-time regression is likely not related to MemorySSA, but the fact that you moved the AST construction before the !Preheader->getParent()->hasProfileData() check, and the profitability heuristic. This means that expensive AST construction now happens every time, instead of only when useful.

@nikic, this is strange because it is off by default. I've already reverted the revert, so I will check the time differences between the relanding and if it shows a regression again, I will pull it.

@nikic, saw your second comment after I added mine. I will revert it and take a look.

jamieschmeiser edited the summary of this revision. (Show Details)

@nikic I have moved the test for profile data before the calculation of the alias sets.