Comparing operands should not require the same ValueID

[• jrkoenig]

Summary

When comparing basic blocks, there is an additional check that two
Value*'s should have the same ID, which interferes with merging equivalent
constants of different kinds (such as a ConstantInt and a ConstantPointerNull
in the included testcase). The cmpValues function already ensures that the
two values in each function are the same, so removing this check should not
cause incorrect merging.

Also, the type comparison is redundant, based on reviewing the code and
testing on the test suite and several large LTO bitcodes.

[jfb]

test/Transforms/MergeFunc/merge-const-ptr-and-int.ll

3

Could you add:

• An equivalent negative test, where i32 and i32* are returned with the same p:64 datalayout. Those shouldn't be merged.
• A test where ret i64 1 isn't merged with ret i64* null.
• Test where other equivalent types are merged (vector, array, structure).
• A test where undef of different types is merged.
• A test where endianness would be relevant.

5

"differ"

[• jrkoenig]

Added more positive/negative tests.

[jfb]

test/Transforms/MergeFunc/merge-const-ptr-and-int.ll

4

Missing undef and endian.

28

Here you test A and B funcs by calling them, whereas below you check whether functions are still defined. You should be consistent!

[• jrkoenig]

test/Transforms/MergeFunc/merge-const-ptr-and-int.ll

4

Added 1,2, and 3. For 3, only vectors are considered bitcast-able, and thus the mergable.

For 4, this is not possible. In fact, two constants with the same type, one undef and the other not, will not be merged, because they cannot compare equal.

Consider f() { return -1; }, g() { return undef; } and h() { return 1;}. If undef was the same as any constant, the ordering would not be a total order because f == g and g == h, but f < h. In order to handle this, the undef would need to be turned into one of the constants -1 or 1, but it can't be both at once. This would have to be an in-place mutation in the comparison, which is quite ugly.

For 5, I don't think such a thing is possible. The only way endianness can be exposed is by converting a <2x i32> into a < 8 x i8 >, or some other equivalent conversion. This is allowed by cmpConstants. But any extraction from that vector/operation on it will have a different type, and thus compare differently, so practically I doubt you can construct a program which depends on endianess.

[jfb]

test/Transforms/MergeFunc/merge-const-ptr-and-int.ll

4

For undef you can at least merge two functions of different types, but where both are undef?

28

Missing this :)

[• jrkoenig]

Added undef tests; use consistent merge-checking method

[• jrkoenig]

Formatting fixups.

[jfb]

Ugh... sorry for more comments... I think you should separate each test into its own .ll file: as is they check that merging occurred, but the tests doesn't validate *what* got merged! Having them in separate files would make that obvious and potentially catch tricky bugs.

I'm good after that!

test/Transforms/MergeFunc/merge-const-ptr-and-int.ll

2

Bikeshed: CHECKNOT-NOT is a bit weird. MERGED-NOT would be nicer.

[• jrkoenig]

Split tests into different files.

[jfb]

lgtm

lib/Transforms/IPO/MergeFunctions.cpp

if (GEPL && GEPR) {
         return Res;
       assert(InstL->getNumOperands() == InstR->getNumOperands());
 
       for (unsigned i = 0, e = InstL->getNumOperands(); i != e; ++i) {
         Value *OpL = InstL->getOperand(i);
         Value *OpR = InstR->getOperand(i);
         if (int Res = cmpValues(OpL, OpR))
           return Res;
-        if (int Res = cmpNumbers(OpL->getValueID(), OpR->getValueID()))
-          return Res;
-        // TODO: Already checked in cmpOperation
-        if (int Res = cmpTypes(OpL->getType(), OpR->getType()))
-          return Res;
+        // cmpValues should ensure this is true.
+        assert(cmpTypes(OpL->getType(), OpR->getType()) == 0);
       }
     }
 
     ++InstL, ++InstR;
   } while (InstL != InstLE && InstR != InstRE);
 
   if (InstL != InstLE && InstR == InstRE)
     return 1;

test/Transforms/MergeFunc/merge-const-ptr-and-int.ll

+; RUN: opt -mergefunc -S < %s | FileCheck %s
+; RUN: opt -mergefunc -S < %s | FileCheck -check-prefix=CHECKNOT %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+
+; Afunc and Bfunc differ only in that one returns i64, the other a pointer.
+; These should be merged.
+define internal i64 @Afunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal i64 @Afunc
+  store i32 4, i32* %P
+  store i32 6, i32* %Q
+  ret i64 0
+}
+
+define internal i64* @Bfunc(i32* %P, i32* %Q) {
+; CHECKNOT-NOT: @Bfunc
+  store i32 4, i32* %P
+  store i32 6, i32* %Q
+  ret i64* null
+}
+
+; ==========
+; Cfunc and Dfunc differ only in that one returns i64, the other a pointer, and
+; both return undef. They should be merged. Note undef cannot be merged with
+; anything else, because this implies the ordering will be inconsistent (i.e.
+; -1 == undef and undef == 1, but -1 < 1, so we must have undef != <any int>).
+define internal i64 @Cfunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal i64 @Cfunc
+  store i32 4, i32* %P
+  store i32 6, i32* %Q
+  ret i64 undef
+}
+
+define internal i64* @Dfunc(i32* %P, i32* %Q) {
+; CHECKNOT-NOT: @Dfunc
+  store i32 4, i32* %P
+  store i32 6, i32* %Q
+  ret i64* undef
+}
+
+
+; ==========
+; These should not be merged, as the datalayout says a pointer is 64 bits. No
+; sext/zext is specified, so these functions could lower differently.
+define internal i32 @Ffunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal i32 @Ffunc
+; CHECK-NEXT: store
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
+  store i32 1, i32* %P
+  store i32 3, i32* %Q
+  ret i32 0
+}
+
+define internal i64* @Gfunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal i64* @Gfunc
+; CHECK-NEXT: store
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
+  store i32 1, i32* %P
+  store i32 3, i32* %Q
+  ret i64* null
+}
+
+; ==========
+; These should not be merged either, as 1 != 0.
+define internal i64 @Ifunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal i64 @Ifunc
+; CHECK-NEXT: store
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
+  store i32 10, i32* %P
+  store i32 10, i32* %Q
+  ret i64 1
+}
+
+define internal i64* @Jfunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal i64* @Jfunc
+; CHECK-NEXT: store
+; CHECK-NEXT: store
+; CHECK-NEXT: ret
+  store i32 10, i32* %P
+  store i32 10, i32* %Q
+  ret i64* null
+}
+
+; ==========
+; Merging should still work even if the values are wrapped in a vector.
+define internal <2 x i64> @Mfunc(i32* %P, i32* %Q) {
+; CHECK-LABEL: define internal <2 x i64> @Mfunc
+  store i32 1, i32* %P
+  store i32 1, i32* %Q
+  ret <2 x i64> <i64 0, i64 0>
+}
+
+define internal <2 x i64*> @Nfunc(i32* %P, i32* %Q) {
+; CHECKNOT-NOT: @Nfunc
+  store i32 1, i32* %P
+  store i32 1, i32* %Q
+  ret <2 x i64*> <i64* null, i64* null>
+}