RFC: Mark BasicAA as a CFG-only pass.

Hi,

I’d like to understand if it makes sense to keep BasicAA as a not CFG-only pass, or if it can be updated to CFG-only. The change was made in D44564.

From what I gathered the motivation was PhiValuesAnalysis not being properly updated, and BasicAA having an instance of it.
PhiValuesAnalysis now uses callback values to invalidate deleted values (r340613), PhiValuesAnalysis is also being updated in MemDepAnalysis (D48489) and BasicAA is invalidated if PhiValuesAnalysis gets invalidated.

I may not have the full context here, so I’d like some feedback: does it make sense to make BasicAA a CFG-only pass again?

Thank you,
Alina

Hi,

Here’s a tentative patch of the changes for this: D74353.

Thank you,
Alina

Hi,

Here's a tentative patch of the changes for this: D74353 <https://reviews.llvm.org/D74353&gt;\.

I suppose that, as expected, it's invalidated less often this way. Given that it's generally stateless, does this really represent a cost savings?

-Hal

Hi,

Here’s a tentative patch of the changes for this: D74353.

I suppose that, as expected, it’s invalidated less often this way. Given that it’s generally stateless, does this really represent a cost savings?

-Hal

I don’t think there is a cost savings now, the current patch shouldn’t have much or any impact.
But I think this can affect future infrastructure changes.
I got here by looking at MemCpyOpt, as part of a longer term goal of having the pass sequence [GVN, MemCpyOpt, DSE] use MemorySSA instead of MemDepAnalysis. Currently we’re explicitly checking that, after MemCpyOpt, BasicAA is freed. This wouldn’t make sense when using MemorySSA in all 3 passes, as we’d end up rebuilding MemorySSA due to invalidating BasicAA.
I kept the changes needed to MemCpyOpt to preserve passes separate from this discussion.
Does this clarify some of the motivation?

Alina

    Hi,

    Here's a tentative patch of the changes for this: D74353
    <https://reviews.llvm.org/D74353&gt;\.

    I suppose that, as expected, it's invalidated less often this way.
    Given that it's generally stateless, does this really represent a
    cost savings?

     -Hal

I don't think there is a cost savings now, the current patch shouldn't have much or any impact.
But I think this can affect future infrastructure changes.
I got here by looking at MemCpyOpt, as part of a longer term goal of having the pass sequence [GVN, MemCpyOpt, DSE] use MemorySSA instead of MemDepAnalysis. Currently we're explicitly checking that, after MemCpyOpt, BasicAA is freed. This wouldn't make sense when using MemorySSA in all 3 passes, as we'd end up rebuilding MemorySSA due to invalidating BasicAA.
I kept the changes needed to MemCpyOpt to preserve passes separate from this discussion.
Does this clarify some of the motivation?

Yes, please add this information to the patch description.

I think that it makes sense to mark BasicAA as not being invalidated on non-CFG-altering IR changes if it's not.

-Hal

Got it, I updated the patch. Please let me know fi I should detail it further.

Thank you,
Alina