[PATCH] D12302: Comparing operands should not require the same ValueID

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: test/Transforms/MergeFunc/merge-const-ptr-and-int.ll:3
@@ +2,3 @@
+; 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"
+
----------------
jrkoenig wrote:
> jfb wrote:
> > Missing `undef` and endian.
> 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.
For `undef` you can at least merge two functions of different types, but where both are `undef`?

================
Comment at: test/Transforms/MergeFunc/merge-const-ptr-and-int.ll:27
@@ +26,3 @@
+  call i64* @Bfunc(i32* %P, i32* %Q)
+  call i64 @Afunc(i32* %Q, i32* %P)
+  ret void
----------------
jfb wrote:
> Here you test A and B funcs by calling them, whereas below you check whether functions are still defined. You should be consistent!
Missing this :)


http://reviews.llvm.org/D12302