Page MenuHomePhabricator

[MSSA] Extending IsGuaranteedLoopInvariant to support an instruction defined in the entry block
ClosedPublic

Authored by fvrmatteo on Feb 21 2021, 8:56 AM.

Details

Summary

As mentioned in D96979, I'm extending the IsGuaranteedLoopInvariant check also to the MemorySSA.cpp file.

@fhahn For now I didn't unify the function into MemorySSA.h because, as you mentioned, it's not directly MSSA related. I'm open to suggestions to find a better place so we can improve the unification process.

I was also wondering if it would make sense to have a utility function called isEntryBlockInstruction (part of llvm::Instruction) to avoid the ugly check used in this (and the mentioned) patch.

I didn't provide a test yet, because it's not clear to me how to create one that is showing the improvement in the MemorySSA case.

Diff Detail

Event Timeline

fvrmatteo created this revision.Feb 21 2021, 8:56 AM
fvrmatteo requested review of this revision.Feb 21 2021, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2021, 8:56 AM
fhahn added a comment.Feb 22 2021, 1:29 PM

I didn't provide a test yet, because it's not clear to me how to create one that is showing the improvement in the MemorySSA case.

Could you add a variant of the test added in a0017c2bc258690146f18491317144e487ddb101 ?

I didn't provide a test yet, because it's not clear to me how to create one that is showing the improvement in the MemorySSA case.

Could you add a variant of the test added in a0017c2bc258690146f18491317144e487ddb101 ?

I tried, but even though I see the new code being triggered and successfully returning true when expected to do so, I can't seem to notice any difference when printing the MemorySSA information (with -aa-pipeline=basic-aa -passes='print<memoryssa>,verify<memoryssa>'). So I'm still clueless on what should be the expected testable behaviour.

fvrmatteo updated this revision to Diff 328539.Mar 5 2021, 8:35 AM

Added a unit test to verify that the modification to IsGuaranteeLoopInvariant leads to retaining the location information associated to a MemoryAccess (in this case a MemoryPhi) when handling a pointer defined in the entry block.

Added a unit test to verify that the modification to IsGuaranteeLoopInvariant leads to retaining the location information associated to a MemoryAccess (in this case a MemoryPhi) when handling a pointer defined in the entry block.

Thanks for adding the test!

Is it possible to have a .ll test instead of the unit test? If we need a unit test, can we at least parse the IR from a string, to make it a bit easier to see what we are working with (e.g. like in https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Analysis/ValueTrackingTest.cpp#L115)

I couldn't manage to craft a test based on an .ll file, so I kept the unit test, but I'm now parsing the IR from a string.

fhahn accepted this revision.Mar 14 2021, 2:30 PM

LGTM thanks. As mentioned at an earlier review, it would be good to unify the implementation with the one DSE uses.

This revision is now accepted and ready to land.Mar 14 2021, 2:30 PM
fhahn added a comment.Mar 23 2021, 2:21 PM

I forgot to commit this for you. Let me do that now.