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

[JF Bastien via llvm-commits]

jfb added a comment.

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

> In http://reviews.llvm.org/D19075#406978, @jfb wrote:
>
> > 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?
>
>
> I'm asking for the general solution first, specifically because it seems like it would be simpler than the current code, and more general. I really don't want to embed specific knowledge of the format of the loop metadata into this pass if not really necessary.


Ah OK, thanks! I'll take a stab at this and send another patch.


http://reviews.llvm.org/D19075