[PATCH] D7864: This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h

[Daniel Berlin via llvm-commits]

On Thu, Jan 7, 2016 at 4:48 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> On Thu, Jan 7, 2016 at 4:27 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
>> hfinkel added inline comments.
>>
>> ================
>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1124
>> @@ +1123,3 @@
>> +  auto CurrAccessPair = UpwardsBFSWalkAccess(StartingAccess, Prev, Q);
>> +  // Either we will have found something that conflicts with us, or we
>> will have
>> +  // hit the liveOnEntry. Check for liveOnEntry.
>> ----------------
>> This algorithm in getClobberingMemoryAccess does not seem correct.
>
>
>
>> If we have this:
>>
>>      [ entry ]
>>          |
>>         ...
>>          |
>>      (clobbering access - a)
>>         ...
>>     /          \
>>   ...         (clobbering access - b)
>>    |              |
>>   ...           ....
>>    \              /
>>           ...
>>    (starting access)
>>
>> UpwardsBFSWalkAccess seems to walk upward until it finds its first
>> clobbering access, and returns that when found. In the case above, if we
>> find clobbering access (b) first, then we're okay, because we'll pick some
>> dominating def farther down the path which is on all paths to both
>> dominating accesses.
>
>
>> But, if we hit clobbering access (a) first, then we'll accept that
>> (incorrectly) as the new final access, because it dominates the original.
>> However, not all paths to all aliasing defs terminate (or pass through)
>> that access.
>>
>
>
> So, note, in the above case,  there *must* be a memory phi in the block of
> starting access, merging the two memory accesses. We should say that is the
> clobbering access.
>
>
> But you are correct, that this version, in the middle of refactoring, did
> not finish this rule correctly. The original algorithm only walked past a
> phi if all paths away from the phi reached the same clobbering access.
>
> This one should do the same.
>

(You can see this in the upwardsdfswalk code that got removed:

  // All the phi arguments should reach the same point if we can bypass
-        // this phi.  The alternative is that they hit this phi node, which
-        // means we can skip this argument.
-        if (FirstDef && (CurrentPair.first != PHIPair.first &&
-                         FirstDef != CurrentPair.first)) {
-          ModifyingAccess = CurrAccess;
-          break;
-        } else if (!FirstDef) {
-          FirstDef = CurrentPair.first;
)




>
>
>>   [entry]
>>     |
>>   .....
>>   (clobbering access - b)
>>    |
>>   ....   ________________________________
>>     \   /                                                               |
>>      (x)                                                               |
>>    ......                                                               |
>>       |                                                                 |
>>       |    ______________________                 |
>>        \   /                                        |                   |
>>   (starting access)                        |                   |
>>        ...                                           |                   |
>>    (clobbering access - a)              |                   |
>>       ...                                            |                   |
>>       | |                                            |                   |
>>       | |______________________|                   |
>>       |                                                                  |
>>       |_________________________________|
>>
>>
>
> Again, we will have a memory phi node here, and we will say the phi node
> is the clobbering access. We will walk one step from the phi, hit a, and
> say "we have a clobbering access."
>

and by here i mean, you will have a memoryphi node in the block of
startingaccess, and we should say that is the clobbering node.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160107/ecbc0f0f/attachment.html>