[PATCH] D58560: [AST] Set 'MayAlias' instead of 'MustAlias' in AliasSets for PHI nodes (bugzilla bug#36801)

Wed Feb 27 14:05:06 PST 2019

asbirlea added a comment.

This is a pretty tricky one, and I may be missing some subtleties here.

[ Code description below refers to `SSAUpdater::RewriteUse` called from `LoopRotationUtils::RewriteUsesOfClonedInstructions` ]

The issue I see is that we're removing a Phi and adding another phi outside the loop. So there's now this user in the loop exit block:
`%p.0.lcssa = phi i32* [ %p.0, %for.cond ], [ %p.0, %entry ]`, where its argument is `%p.0 = phi i32* [ null, %entry ], [ %arrayidx, %for.end ]`
The SSAUpdater is saying "//notify values that I replaced all uses of %p.0 with null//", then "//notify values that I replaced all uses of %p.0 with %arrayidx//" (`SSAUpdater::RewriteUse:202`).
In fact, only one of the entries in `%p.0.lcssa` is replaced each time, leading to a correct phi node: `%p.0.lcssa = phi i32* [ %arrayidx, %for.cond ], [null, %entry ]`.
There are two distinct `Use`s of the same `Value`. Each is replaced with a different `Value`. I'm inclined to say calling `ValueHandleBase::ValueIsRAUWd` is not always correct here. But perhaps the fix should be in the VH?

Now, this example is running a sequence of LoopRotate, LICM, LoopRotate, LICM. The issue is that the info computed by the first LICM is incorrectly updated in LoopRotate and that incorrect result is used in the second LICM.

LICM builds an AliasSetTracker on the first run and caches it. Then LoopRotate calls the above magic using the SSAUpdater, "informing" the cached AliasSetTracker first that `%p.0 ` is replaced with `null` and then that `%p.0 ` is replaced with `%arrayidx` (the mechanism is in `ASTCallbackVH`). So the AliasSetTracker will happily create a MustSet with `null` and `%arrayidx`. That's clearly wrong. So, one solution is to check that phis are handled differently and recheck alias info, as this patch proposes. This way we push the fix in the VH.
But, I thought the original intent of `copyValue` was to guarantee the value replaced/cloned is essentially the same, hence it should MustAlias. This call is breaking that assumption. If I got this assumption wrong, then feel free to correct me.

Alternative solutions are to fix the callsite or to not cache the AliasSetTracker (fwiw, not caching resolves this issue).
I'm not sure how to fix the callsite. For the AST, a single `copyValue(%p.0, %p.0.lcssa)` is probably correct, but it can't be done given the current codebase. Suggestions welcome.

I don't have anything against this change, and, considering this is a current miscompile, I'm ok to be checked in as is. But AFAICT, the issue is somewhat more complex.
I'll be happy if folks more familiar with the SSAUpdater can chime in.

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

https://reviews.llvm.org/D58560