[PATCH] D67612: [UnrolledInstAnalyzer] Use MSSA to find stored values outside of loop.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

In D67612#1671599 <https://reviews.llvm.org/D67612#1671599>, @fhahn wrote:

> In D67612#1671530 <https://reviews.llvm.org/D67612#1671530>, @asbirlea wrote:
>
> > Here are a couple of answers/discussion points:
> >
> > First of, could you please elaborate a bit more on what you're trying to check for? Is it stores before the loop, after the loop or both?
>
>
> The idea here is to check stores before the loop and use the store to determine the loaded value in a specific loop iteration. We only do that if we can determine a base pointer + constant offset for a certain loop iteration.


This is the part I do not understand. It looks like the `LoadInst` is considered "free" and not marked towards the cost of the unroll if there is a store with the same base pointer + offset.
Is that always the case? If there is an interfering MemoryAccess between the two (the walker's answer), then the `LoadInst` is not free. 
I'm not that familiar with the code here, so feel free to correct me if I misunderstood.

> 
> 
>> Normally we'd use the walker on the instruction (`MSSA->getWalker()->getClobberingMemoryAccess(&I)`) to look before the loop. Ideally we'd avoid going into the innards of MSSA in loop passes, so if a usecase not covered by the walker is needed that should be added either to a walker or to the updater.
>>  It looks like a good use of MemorySSA, but the search should happen behind an API.
> 
> Right, but the walker just does a single step, which might not be the defining instructions we are looking for. Would the recommended way to implement this to implement a new walker, which just looks for a MemDef with said base pointer + offset?

Yes, if this is your goal, having a new walker do this would be the way to go.

> 
> 
>> Secondly, in the new pass manager it is necessary for MSSA to be preserved by all loop passes in the same loop pipeline, because analyses are not actually invalidated until the entire loop pipeline is done. Hence it's possible to end up using an invalidated analysis and only notice this on a crash, miscompile or non-deterministic behavior. This occurred in D65060 <https://reviews.llvm.org/D65060> and I tried to add a safety net in D66376 <https://reviews.llvm.org/D66376> to prevent this from happening again (LPM2 will not use MSSA even if LoopUnroll is taught to preserve it,  until all other passes in LPM2 all preserve it).
>> 
>> As far as preserving MemorySSA in LoopUnroll, this is a complex task. I had started work in this, but I did not complete it because unrolling does a lot of cloning/copying, and adding instructions "out of nowhere" to MSSA can lead to complex updates. It may be worth evaluating if updating MSSA is worth it for the unroll pass, or if it's faster to rebuild it from scratch. I did not get a chance to make this evaluation.
> 
> Right, I expect this to be a fair bit of work, but I'd be happy to look into it as pre-requisite for this patch.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67612/new/

https://reviews.llvm.org/D67612