This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by aqjune on Jun 29 2017, 7:09 PM.

Details

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 .

Diff Detail

Repository
rL LLVM

Event Timeline

aqjune created this revision.Jun 29 2017, 7:09 PM
mgrang added a subscriber: llvm-commits.
mgrang added a subscriber: mgrang.

Please add unit test(s) for your patch.

rjmccall edited edge metadata.Jun 29 2017, 9:33 PM

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

aqjune updated this revision to Diff 104822.Jun 29 2017, 11:37 PM

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

chenwj added a subscriber: chenwj.EditedJun 30 2017, 5:58 AM

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?

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

rjmccall accepted this revision.Jun 30 2017, 9:51 AM

Looks great, thanks.

This revision is now accepted and ready to land.Jun 30 2017, 9:51 AM
aqjune added a comment.Jul 3 2017, 4:39 AM

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

aqjune added a comment.Jul 7 2017, 7:47 PM

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 added a comment.EditedJul 9 2017, 6:19 PM

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 updated this revision to Diff 105814.Jul 10 2017, 12:46 AM

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

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

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.

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!

This revision was automatically updated to reflect the committed changes.