[PATCH] D19075: mergefunc: avoid merge with loop metadata

[JF Bastien via llvm-commits]

jfb added a comment.

In http://reviews.llvm.org/D19075#406966, @hfinkel wrote:

> In http://reviews.llvm.org/D19075#406955, @jfb wrote:
>
> > In http://reviews.llvm.org/D19075#406907, @hfinkel wrote:
> >
> > > In http://reviews.llvm.org/D19075#406897, @jfb wrote:
> > >
> > > > In http://reviews.llvm.org/D19075#405855, @hfinkel wrote:
> > > >
> > > > > Do we not unqiue MDNodes? This seems like a lot of code for what I would have thought should be: Compare the pointer equality all MDNode operands except the first (because the first is the unique loop id).
> > > >
> > > >
> > > > mergefunc wants an ordering for what it compares, so pure equality isn't sufficient (although `cmpLoopOperandMetadata` does start with `L == R`). It needs to know how to order in a stable way, so we also can't just order based on pointer value.
> > >
> > >
> > > Do you not already have a way to do this for metadata in general?
> >
> >
> > Not that I know, but I've been wrong before! I think a lot of the logic in mergefunc could be moved to the actual instruction (ordering as well as mergefunc's special hashing) and factored in a way that would reduce the risk of breakage (right now mergefunc breaks if not updated in sync).
>
>
> This synchronization requirement is part of what worries me about embedding so much knowledge of the metadata format in this code.
>
> > I need to hack on things a bit more before I feel comfortable enough to propose a clean solution.
>
>
> Okay. I suspect a general solution for metadata can be used here (minus the first operand for loop ids), once such a thing exists.


Are you saying the current patch looks good with promise to generalize solution later, or are you asking for the generalized solution first?


http://reviews.llvm.org/D19075