This is an archive of the discontinued LLVM Phabricator instance.

[PM][InstCombine] fixing omission of AliasAnalysis in new-pass-manager's version of InstCombine
ClosedPublic

Authored by fedor.sergeev on Dec 13 2017, 1:58 PM.

Details

Summary

Passing AliasAnalysis results instead of nullptr appears to work just fine.
A couple new-pass-manager tests updated to align with new order of analyses.

Event Timeline

fedor.sergeev created this revision.Dec 13 2017, 1:58 PM

removing invalid comment

chandlerc accepted this revision.Dec 13 2017, 2:25 PM

LGTM. It'd be great to add an instcombine test that uses new PM and requires AA to optimize as well. Can be in a follow-up commit if needed.

This revision is now accepted and ready to land.Dec 13 2017, 2:25 PM
skatkov added inline comments.
lib/Transforms/InstCombine/InstructionCombining.cpp
3289

Chandler, may be you can answer this question: why do we need to preserve BasicAA if we already preserved AAManager?

chandlerc added inline comments.Dec 13 2017, 10:57 PM
lib/Transforms/InstCombine/InstructionCombining.cpp
3289

The AA manager just brokers access to the various different alias analyses. You still need to preserve the analyses themselves.

Now that the invalidation support is fully wired up, we could probably remove the need to explicitly preserve the AAManager and just have it remain preserved automatically if all of the AAs are preserved. But it isn't a big deal. The only cost to invalidating the AAManager is that we have to go and look up the N different alias analyses for the next pass.

Thank you for explanation!

What is the valid way to preserve all alias analysis?

adding a test

Ahem, could somebody ok the test, pls?

This revision was automatically updated to reflect the committed changes.