This is an archive of the discontinued LLVM Phabricator instance.

mergefunc: avoid merge with loop metadata
AbandonedPublic

Authored by jfb on Apr 13 2016, 2:26 PM.

Details

Reviewers
rengolin
hfinkel
Summary

Loop metadata hints at unrolling and vectorization. mergefunc shouldn't merge
when they're different since someone expressed the wish for the functions to
differ (yes we could also ignore metadata, but let's not be mean). This can
happen when running mergefunc 'early' (right after clang or another frontend
generates code) but shouldn't happen when running it 'late' (after optimizations
are run and loop metadata has been put to use).

This inadvertently happens in test/Transforms/LoopVectorize/X86/metadata-enable.ll

This patch doesn't fix all of mergefunc's handling of metadata, it simply makes
one case less broken and puts us closer to being able to use mergefunc.

Diff Detail

Event Timeline

jfb updated this revision to Diff 53621.Apr 13 2016, 2:26 PM
jfb retitled this revision from to mergefunc: avoid merge with loop metadata.
jfb updated this object.
jfb added reviewers: hfinkel, rengolin.
jfb added a subscriber: llvm-commits.
hfinkel edited edge metadata.Apr 19 2016, 3:39 PM

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

jfb added a comment.Apr 20 2016, 12:50 PM

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.

In D19075#406897, @jfb 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?

jfb added a comment.Apr 20 2016, 1:22 PM
In D19075#406897, @jfb 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). I need to hack on things a bit more before I feel comfortable enough to propose a clean solution.

In D19075#406955, @jfb wrote:
In D19075#406897, @jfb 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.

jfb added a comment.Apr 20 2016, 1:43 PM
In D19075#406955, @jfb wrote:
In D19075#406897, @jfb 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?

In D19075#406978, @jfb wrote:
In D19075#406955, @jfb wrote:
In D19075#406897, @jfb 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.

jfb added a comment.Apr 20 2016, 1:53 PM
In D19075#406978, @jfb wrote:
In D19075#406955, @jfb wrote:
In D19075#406897, @jfb 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.

rengolin edited edge metadata.Dec 12 2016, 1:46 AM

Was this abandoned?

jfb abandoned this revision.May 7 2018, 9:58 PM