[PATCH] D154119: Fix: Distinguish CFI Metadata Checks in MergeFunctions Pass

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

Sorry, Phabricator lost my comment somehow. Adding back a version of it now.

I don't think it's viable to have the two functions arbitrarily compare "less than" if they both have distinct metadata. That leads to assertions like this:

  assert(!(V1<V2 && V2<V1) && "Both values are less than the other?");

firing.

It also makes it undefined behaviour to use the comparator in "sort" algorithms. Have a look at:
https://en.cppreference.com/w/cpp/named_req/LessThanComparable
https://en.wikipedia.org/wiki/Weak_ordering#Strict_weak_orderings

And I assume this is used for sorting somehow?

I suggest changing function merging to check for distinct metadata as a second step, after functions compare equal. But there might be another way.



================
Comment at: llvm/lib/Transforms/Utils/FunctionComparator.cpp:810
+          return 0;
+        return -1;
+      }
----------------
This breaks strict weak ordering, which means compareValues can never be used for sorting, which I assume is the whole point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154119/new/

https://reviews.llvm.org/D154119