Let llvm-diff correctly deal with Undef/ConstantAggregateZero/ConstantVector/IndirectBr

[aqjune]

Summary

llvm-diff incorrectly reports that there's a diff when input IR contains undef/zeroinitializer/constantvector/indirectbr.
(This happens even if two identical files are given, e.g. llvm-diff x.ll x.ll)

This is fix to the bug report https://bugs.llvm.org/show_bug.cgi?id=33623 .

[mgrang]

Please add unit test(s) for your patch.

[rjmccall]

Yeah, needs tests. The logic seems fine to me.

[aqjune]

I added 4 tests, and checked that make check-llvm works well.

[chenwj]

In D34856#796437, @aqjune wrote:

I added 4 tests, and checked that make check-llvm works well.

@aqjune Is there a way to decide llvm-diff pass or fail? Are those test cases fail in absence of your patch?

[aqjune]

@chenwj Yes, these tests failed in absence of my patch.

[rjmccall]

Looks great, thanks.

[aqjune]

Hello, I don't have commit access. Could someone commit this patch instead? Thanks.

[aqjune]

Hello, how can I commit this patch to SVN? I don't think I have commit access to SVN. :( If someone applies this patch instead of me it would be a great help. (I wrote a comment here according to the policy written in 'Commiting a change', http://llvm.org/docs/Phabricator.html )

[mgrang]

I tried committing this patch but ran into merge conflicts. Can you please check?

Current branch master is up to date.
Created and checked out branch arcpatch-D34856.
Checking patch tools/llvm-diff/DifferenceEngine.cpp...
Checking patch test/tools/llvm-diff/zeroinitializer.bc.ll...
error: test/tools/llvm-diff/zeroinitializer.bc.ll: does not exist in index
Checking patch test/tools/llvm-diff/undef.ll...
error: test/tools/llvm-diff/undef.ll: does not exist in index
Checking patch test/tools/llvm-diff/indirectbr.ll...
error: test/tools/llvm-diff/indirectbr.ll: does not exist in index
Checking patch test/tools/llvm-diff/constantvector.ll...
error: test/tools/llvm-diff/constantvector.ll: does not exist in index
Applied patch tools/llvm-diff/DifferenceEngine.cpp cleanly.

 Patch Failed!
Usage Exception: Unable to apply patch!
M       tools/llvm-diff/DifferenceEngine.cpp
Switched to branch 'master'
Already up-to-date.

[aqjune]

I apologize for my mistake. I updated the diff and checked that it works with patch -p0 -i diff.patch

[mgrang]

I am still getting the same error. Maybe someone else can try.

[aqjune]

Could someone have a try to apply this patch? I just checked out llvm from SVN, tried patch -p0 -i ../patch.file, and it worked well.

[modocache]

Thanks for working on this, @aqjune! I was able to apply the patch cleanly, and all the tests build and passed, so I committed it as rL329957. Thanks again!

llvm/trunk/test/tools/llvm-diff/constantvector.ll

+; Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=33623
+; RUN: llvm-diff %s %s
+
+%struct.it = type { i64, i64* }
+
+@a_vector = internal global [2 x i64] zeroinitializer, align 16
+
+define i32 @foo(%struct.it* %it) {
+
+entry:
+  %a = getelementptr inbounds %struct.it, %struct.it* %it, i64 0, i32 1
+  %tmp0 = bitcast i64** %a to <2 x i64*>*
+  store <2 x i64*> <i64* getelementptr inbounds ([2 x i64], [2 x i64]* @a_vector, i64 0, i64 0), i64* getelementptr inbounds ([2 x i64], [2 x i64]* @a_vector, i64 0, i64 0)>, <2 x i64*>* %tmp0, align 8
+
+  ret i32 0
+}

llvm/trunk/test/tools/llvm-diff/indirectbr.ll

+; Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=33623
+; RUN: llvm-diff %s %s
+
+define i32 @foo(i8*) {
+entry:
+  indirectbr i8* %0, [label %A, label %B, label %entry]
+A:
+  ret i32 1
+B:
+  ret i32 2
+}

llvm/trunk/test/tools/llvm-diff/undef.ll

+; Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=33623
+; RUN: llvm-diff %s %s
+
+%A = type { i64, i64 }
+@_gm_ = global <2 x %A*> zeroinitializer
+
+define void @f() {
+entry:
+  store <2 x %A*> undef, <2 x %A*>* @_gm_
+  ret void
+}

llvm/trunk/test/tools/llvm-diff/zeroinitializer.bc.ll

+; Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=33623
+; RUN: llvm-diff %s %s
+
+%A = type { i64, i64 }
+@_gm_ = global <2 x %A*> zeroinitializer
+
+define void @f() {
+entry:
+  store <2 x %A*> zeroinitializer, <2 x %A*>* @_gm_
+  ret void
+}

llvm/trunk/tools/llvm-diff/DifferenceEngine.cpp

// Terminators.
           if (Complain) Engine.log("branch conditions differ");
           return true;
         }
         if (TryUnify) tryUnify(LI->getSuccessor(1), RI->getSuccessor(1));
       }
       if (TryUnify) tryUnify(LI->getSuccessor(0), RI->getSuccessor(0));
       return false;
 
+    } else if (isa<IndirectBrInst>(L)) {
+      IndirectBrInst *LI = cast<IndirectBrInst>(L);
+      IndirectBrInst *RI = cast<IndirectBrInst>(R);
+      if (LI->getNumDestinations() != RI->getNumDestinations()) {
+        if (Complain) Engine.log("indirectbr # of destinations differ");
+        return true;
+      }
+
+      if (!equivalentAsOperands(LI->getAddress(), RI->getAddress())) {
+        if (Complain) Engine.log("indirectbr addresses differ");
+        return true;
+      }
+
+      if (TryUnify) {
+        for (unsigned i = 0; i < LI->getNumDestinations(); i++) {
+          tryUnify(LI->getDestination(i), RI->getDestination(i));
+        }
+      }
+      return false;
+
     } else if (isa<SwitchInst>(L)) {
       SwitchInst *LI = cast<SwitchInst>(L);
       SwitchInst *RI = cast<SwitchInst>(R);
       if (!equivalentAsOperands(LI->getCondition(), RI->getCondition())) {
         if (Complain) Engine.log("switch conditions differ");
         return true;
       }
       if (TryUnify) tryUnify(LI->getDefaultDest(), RI->getDefaultDest());
if (isa<GlobalValue>(L))
       return Engine.equivalentAsOperands(cast<GlobalValue>(L),
                                          cast<GlobalValue>(R));
 
     // Compare constant expressions structurally.
     if (isa<ConstantExpr>(L))
       return equivalentAsOperands(cast<ConstantExpr>(L),
                                   cast<ConstantExpr>(R));
 
-    // Nulls of the "same type" don't always actually have the same
+    // Constants of the "same type" don't always actually have the same
     // type; I don't know why.  Just white-list them.
-    if (isa<ConstantPointerNull>(L))
+    if (isa<ConstantPointerNull>(L) || isa<UndefValue>(L) || isa<ConstantAggregateZero>(L))
       return true;
 
     // Block addresses only match if we've already encountered the
     // block.  FIXME: tentative matches?
     if (isa<BlockAddress>(L))
       return Blocks[cast<BlockAddress>(L)->getBasicBlock()]
                  == cast<BlockAddress>(R)->getBasicBlock();
 
+    // If L and R are ConstantVectors, compare each element
+    if (isa<ConstantVector>(L)) {
+      ConstantVector *CVL = cast<ConstantVector>(L);
+      ConstantVector *CVR = cast<ConstantVector>(R);
+      if (CVL->getType()->getNumElements() != CVR->getType()->getNumElements())
+        return false;
+      for (unsigned i = 0; i < CVL->getType()->getNumElements(); i++) {
+        if (!equivalentAsOperands(CVL->getOperand(i), CVR->getOperand(i)))
+          return false;
+      }
+      return true;
+    }
+
     return false;
   }
 
   bool equivalentAsOperands(ConstantExpr *L, ConstantExpr *R) {
     if (L == R)
       return true;
     if (L->getOpcode() != R->getOpcode())
       return false;