[llvm] r198290 - BasicAA: Fix value equality and phi cycles
Nick Lewycky
nicholas at mxc.ca
Mon Jan 13 01:26:06 PST 2014
Arnold Schwaighofer wrote:
>
> On Jan 1, 2014, at 8:24 PM, Nick Lewycky<nicholas at mxc.ca> wrote:
>
>> On 01/01/2014 07:31 PM, Arnold Schwaighofer wrote:
>>> Author: arnolds
>>> Date: Wed Jan 1 21:31:36 2014
>>> New Revision: 198290
>>> // Are we checking for alias of the same value?
>>> - if (V1 == V2) return MustAlias;
>>> + if (isValueEqual(V1, V2)) return MustAlias;
>>
>> This demands a massive explanation of what on earth is going on
>
>> Of course %x mustalias %x! (Yes, I've read the PR and understand it what's going on, but I'd much rather see code that explicitly dealt with the "we're in a loop and speculating on PHIs case" broken out.)
>
> At the top of the file I put the comment:
>
> 42 /// Cutoff after which to stop analysing a set of phi nodes potentially involved
> 43 /// in a cycle. Because we are analysing 'through' phi nodes we need to be
> 44 /// careful with value equivalence. We use dominance to make sure a value cannot
> 45 /// be involved in a cycle.
> 46 const unsigned MaxNumPhiBBsValueDominanceCheck = 20;
>
> The function isValueEqual has the comment:
>
> + /// \brief Check whether two Values can be considered equivalent.
> + ///
> + /// In addition to pointer equivalence of \p V1 and \p V2 this checks
> + /// whether they can not be part of a cycle in the value graph by looking at
> + /// all visited phi nodes an making sure that the value dominates all of
> + /// them.
> + bool isValueEqual(const Value *V1, const Value *V2);
>
> The variable VisitedPhiBBs has the comment:
>
>
> 508 /// \brief Track phi nodes we have visited. When interpret "Value" pointer
> 509 /// equality as value equality we need to make sure that the "Value" is not
> 510 /// part of a cycle. Otherwise, two uses could come from different
> 511 /// "iterations" of a cycle and see different values for the same "Value"
> 512 /// pointer.
> 513 /// The following example shows the problem:
> 514 /// %p = phi(%alloca1, %addr2)
> 515 /// %l = load %ptr
> 516 /// %addr1 = gep, %alloca2, 0, %l
> 517 /// %addr2 = gep %alloca2, 0, (%l + 1)
> 518 /// alias(%p, %addr1) -> MayAlias !
> 519 /// store %l, ...
> 520 SmallPtrSet<const BasicBlock*, 8> VisitedPhiBBs;
>
>
> I can add another comment at isValueEqual's use?
Yes, that's where I wanted the comment.
Given the API that alias analysis has, alias(%x, %x) == mustalias. There
are ways that could be false (analyzing %x across two different call
sites to the same function), but for the subset of alias queries we can
express in the AliasAnalysis API, it has to be true.
Now here we are in a function named aliasCheck(V1, V2) which has always
been attempting to answer the question "does V1 alias V2?". If V1 == V2
does not lead to a mustalias answer, then this function *isn't* actually
answering alias queries any more, it's doing something else, different,
surprising. That's my problem.
This whole VisitedPhiBBs thing doesn't belong in aliasCheck. aliasPHI
needs to check some property of the two pointers that isn't the aliasing
property -- at least, not the aliasing property as defined by LLVM's
AliasAnalysis API. It could be called aliasing in the literature, but
it's some sort of CFG-aware aliasing test. I don't really know what it is.
Nick
More information about the llvm-commits
mailing list