[PATCH] D59442: Enable Operand Reordering for Commutative Instructions in the FunctionComparator/MergeFunctions

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

In D59442#1431789 <https://reviews.llvm.org/D59442#1431789>, @rcorcs wrote:

> This example that I gave in my comment is a simplistic one that gets canonicalized.
>  However, this patch would handle more interesting examples as well. Such as the following one:
>
> [...]
>
> This makes the existing -mergefunc fail, but this patch fixes it.
>
>   I agree that canonicalization steps help a lot, but, of course, that I like the idea of having a more powerful function merger (as a proponent of one).


Gotcha. Have you seen this occur? Which cases? Are there better mergefuncs-independent canonicalizations that would handle such matching? i.e. can we improve canonicalization so that we don't need to do the matching here?

I want to make sure we don't unnecessarily slow down mergefuncs. We might at some point consider a "slower but more powerful" set of comparators, and "fast but good" ones. As I described on the mailing list last week, mergefuncs can help compile time if we run the simple one early, and way later run the complex one.



================
Comment at: lib/Transforms/Utils/FunctionComparator.cpp:780
+        else FinalRes = Res11;
+        if (FinalRes) return FinalRes;
+      } else {
----------------
Please run the added code through clang-format.


================
Comment at: lib/Transforms/Utils/FunctionComparator.cpp:928
 // when possibly merging functions which are the same modulo constants and call
 // targets.
 FunctionComparator::FunctionHash FunctionComparator::functionHash(Function &F) {
----------------
How does your change behave when hashing? Hashing is an important part of mergefuncs avoiding O(n²) behavior.


================
Comment at: unittests/Transforms/Utils/FunctionComparatorTest.cpp:163
+  ret i32 %add
+}
+  )");
----------------
You should test the more complex example you describe as well.
In general you should be testing all the binary ops that LLVM has.


================
Comment at: unittests/Transforms/Utils/FunctionComparatorTest.cpp:172
+  EXPECT_TRUE(AreEqual);
+}
----------------
I don't think you want the tests here, you instead want regular IR tests with FileCheck. See `test/Transforms/MergeFunc/`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59442