[llvm-commits] [pr12979] updated patch

[Rafael Espíndola]

> 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.

> Nuno

Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: application/octet-stream
Size: 16936 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120604/fef17b79/attachment.obj>