This is an archive of the discontinued LLVM Phabricator instance.

Remove Merge Functions pointer comparisons
ClosedPublic

Authored by jrkoenig on Aug 26 2015, 11:45 AM.

Details

Summary

This patch removes two remaining places where pointer value comparisons
are used to order functions: comparing range annotation metadata, and comparing
block address constants. (These are both rare cases, and so no actual
non-determinism was observed from either case).

The fix for range metadata is simple: the annotation always consists of a pair
of integers, so we just order by those integers.

The fix for block addresses is more subtle. Two constants are the same if they
are the same basic block in the same function, or if they refer to corresponding
basic blocks in each respective function. Note that in the first case, merging
is trivially correct. In the second, the correctness of merging relies on the
fact that the the values of block addresses cannot be compared. This change is
actually an enhancement, as these functions could not previously be merged (see
merge-block-address.ll).

There is still a problem with cross function block addresses, in that constants
pointing to a basic block in a merged function is not updated.

This also more robustly compares floating point constants by all fields of their
semantics, and fixes a dyn_cast/cast mixup.

Diff Detail

Event Timeline

jrkoenig retitled this revision from to Remove Merge Functions pointer comparisons.
jrkoenig updated this object.
jrkoenig added reviewers: jfb, dschuff, nlewycky.
jrkoenig added a subscriber: llvm-commits.
jfb edited edge metadata.Aug 27 2015, 2:38 PM

Missing range metadata tests :-)

lib/Transforms/IPO/MergeFunctions.cpp
536

Could you add a TODO that explains how one could merge functions with different range metadata? The comparison could ignore it, and merging would take the most conservative of the ranges (widest interval, taking caution if there's wrapping, and deleting range information if the range becomes full/emtpy, merging pairs, ...).

543

Range metadata has *pairs* of integers. It has at least two, but can have more:
http://llvm.org/docs/LangRef.html#range-metadata

544

No need for the asserts, since getOperand already asserts.

734

Use a range-based for with F->getBasicBlockList().

743

Need a return here for MSVC.

test/Transforms/MergeFunc/merge-block-address-other-function.ll
7

Remove the auto-generated comments and metadata from the tests (e.g. the #0 below).

test/Transforms/MergeFunc/merge-block-address.ll
57

tail call _Z1fi would be clearer.

100

Same on metadata and comments above.

jrkoenig edited edge metadata.

Fixed range metadata comparison. Cleaned up tests.

jrkoenig marked 5 inline comments as done.

Fixed typo & expanded TODO

jfb added a comment.Aug 27 2015, 6:36 PM
In D12376#234543, @jfb wrote:

Missing range metadata tests :-)

Forgot to git add the range metadata tests?

Add multi-segment range test.

(There is already a test/Transforms/MergeFunc/ranges.ll test in the suite.)

jfb accepted this revision.Aug 27 2015, 9:28 PM
jfb edited edge metadata.

lgtm

This revision is now accepted and ready to land.Aug 27 2015, 9:28 PM
This revision was automatically updated to reflect the committed changes.