[PATCH] D25601: DAGCombiner: fix use-after-free when merging consecutive stores

[Nicolai Hähnle via llvm-commits]

On 02.11.2016 18:54, Justin Bogner wrote:
> Nicolai Hähnle <nhaehnle at gmail.com> writes:
>> nhaehnle updated this revision to Diff 76719.
>> nhaehnle added a comment.
>>
>> Keep returning a boolean.
>>
>> Since not all nodes are necessarily merged, the success-path now needs to
>> truncate the StoreNodes vector to just those nodes that were actually
>> removed.
>
> Looks pretty close. A couple of comments below.

Thanks for taking a look!

[snip]
>> -    do {
>> +    for (;;) {
>>        // There can be multiple store sequences on the same chain.
>>        // Keep trying to merge store sequences until we are unable to do so
>>        // or until we merge the last store on the chain.
>> -      bool Changed = MergeConsecutiveStores(ST);
>> +      SmallVector<MemOpLink, 8> StoreNodes;
>> +      bool Changed = MergeConsecutiveStores(ST, StoreNodes);
>
> What do you think of returning the SmallVector instead of the bool, then
> just doing `EverChanged |= !StoreNodes.empty()`? It seems a bit simpler
> to me, and move semantics should mean it's just as efficient.

A SmallVector move cannot be as efficient because it cannot avoid the 
copy in the small case. I don't feel strongly about it though.


>>        EverChanged |= Changed;
>>        if (!Changed) break;
>> -    } while (ST->getOpcode() != ISD::DELETED_NODE);
>> +
>> +      bool ChangedST = false;
>> +      for (const auto &Link : StoreNodes) {
>> +        if (Link.MemNode == ST) {
>> +          ChangedST = true;
>> +          break;
>> +        }
>> +      }
>> +      if (ChangedST)
>> +        break;
>
> I think this can be simplified with any_of. Something like this:
>
>       if (any_of(StoreNodes,
>                  [&ST](const MemOpLink &Link) { return Link.MemNode == ST; }))
>         return;

Will do.

Cheers,
Nicolai

>
>> +    }
>>
>>      if (EverChanged)
>>        return SDValue(N, 0);