[llvm-commits] [pr12979] updated patch

[Nuno Lopes]

>> The patch looks great overall.
>> Just a few comments:
>>
>> +  while (T) {
>> +    PathA.push_back(T);
>> +    T = T->getNumOperands() == 2 ? cast_or_null<MDNode>(T->getOperand(1)) :
>> 0;
>>
>> should be >= 2 there, since TBAA metadata can have 3 arguments. Same thing
>> in the 2nd loop.
>
> Thanks!
>
>> Also, probably it's a good idea to have a  fast exit if A==B.
>> A few comments in the range merging function would be nice :)  e.g. in the
>> loop to merge the ranges in the two ends, etc.  The whole algorithm is
>> non-trivial (to me, at least).
>
> I added some comments.
>
>>
>> Without wanting to delay your patch much farther, I would like to insist in
>> my previous idea on the ConstantRangeUnion ADT (or whatever name you like).
>> What do you think about the following API:
>> - range metadata -> ConstantRangeUnion
>> - ConstantRangeUnion -> range metadata
>> - ConstantRangeUnion:  union, intersection w/ ConstantRangeUnion and
>> ContantRange
>> (and in the future, add support for all the other operations that
>> ConstantRange supports)
>>
>> I can see use of this API here, and in the value propagation pass, for
>> example. I can certainly help (w/ code).
>
> I can also see some possibilities for refactoring:
>
> * The unionWith method could take an outparam saying if the merge was
> exact or not. That would replace the canBeMerged function: Just try
> the merge and keep it if it is exact.
> * We can factor the set of ranges into a class as you suggest.
> * Other passes might make use of the metadata and flag merging.
>
> Having said that, I would like to fix just gvn first if possible as
> the patch is already fairly big and all of the above changes will
> probably have some interesting points to discuss too.

Sure; I think the patch is ready to go in!

Nuno