[llvm] r299851 - [InstCombine] fix matching of or-of-icmps constants (PR32524)
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 13 09:21:17 PDT 2017
Thanks! I have made changes, so we will not need so much effort to see a
simple bug like this next time I try this patch :)
https://reviews.llvm.org/rL300201
https://reviews.llvm.org/rL300202
On Wed, Apr 12, 2017 at 8:27 PM, NAKAMURA Takumi <geek4civic at gmail.com>
wrote:
> On Thu, Apr 13, 2017 at 7:40 AM Sanjay Patel <spatel at rotateright.com>
> wrote:
>
>> Thank you for finding the file. Can you share how you knew the difference
>> was in there?
>>
>
> FYI, ;)
>
> * Assumptions
> - Changes (in the compiler) should not be affected to entire generated
> code, at most hundreds of object files.
> - Changes should not break ABI.
>
> * Steps
> 0. Do not use Ninja for stage2. Ninja eventually rebuild and overwrite
> object files even if their timestamp is up-to-date.
> (Older version of ninja could be available. It didn't use deps db.)
> Use Makefile generator for stage2.
> 1. Build stage1 clang with the good revision and build the tree with it
> as stage2. Confirm stage2 fine.
> 2. Add whole object files to the git blob. In that case, I guessed I
> didn't need to track utils/, tools/ and tools/clang.
> $ git init
> $ find lib -type f -name '*.o' | xargs git add
> $ git commit -mgood
> 3. Build stage1 clang again in a dubious revision and build the stage2
> tree from clean. Confirm stage2 reproduces failures.
> 4. Commit changed files identically.
> $ find lib -type f -name '*.o' -exe git commit -mOBJ-{} {} \;
> 5. Play git-bisect, and move a dubious commit into tail with git-rebase
> -i, without cleaning stage2.
> Rebuilding is just doing archiving, linking and testing w/o
> rebuilding object files.
>
> Then, you may focus investigating the culprit compile unit.
>
> The way would be applicable also to find miscompilations between clang and
> gcc, as far as they are ABI-compatible.
> I have no idea how it could be full-automated.
>
>
> I can see the bug now starts with this existing line of code:
>> assert(LHSC->getValue().ule(LHSC->getValue()));
>>
>> We asserted that a value was equal to itself. :)
>>
>> On Tue, Apr 11, 2017 at 10:00 AM, NAKAMURA Takumi <geek4civic at gmail.com>
>> wrote:
>>
>> ATM, I found the difference in AMDGPUBaseInfo.cpp causes tests failures.
>>
>>
>> On Wed, Apr 12, 2017 at 12:39 AM Sanjay Patel <spatel at rotateright.com>
>> wrote:
>>
>> Thank you for letting me know. I am curious to know how to debug this.
>>
>> On Tue, Apr 11, 2017 at 8:36 AM, NAKAMURA Takumi <geek4civic at gmail.com>
>> wrote:
>>
>> Seems it causes miscompilation (or revealing a bug) in AMDGPUCodeGen, in
>> stage 2.
>> Investigating.
>>
>> http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/14928
>>
>> On Tue, Apr 11, 2017 at 2:08 AM Sanjay Patel via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>> Author: spatel
>> Date: Mon Apr 10 11:55:57 2017
>> New Revision: 299851
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=299851&view=rev
>> Log:
>> [InstCombine] fix matching of or-of-icmps constants (PR32524)
>>
>> Also, make the same change in and-of-icmps and remove a hack for
>> detecting that case.
>>
>> Finally, add some FIXME comments because the code duplication here is
>> awful.
>>
>> This should fix the remaining IR problem noted in:
>> https://bugs.llvm.org/show_bug.cgi?id=32524
>>
>>
>> Modified:
>> llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> llvm/trunk/test/Transforms/InstCombine/or.ll
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
>> Transforms/InstCombine/InstCombineAndOrXor.cpp?rev=
>> 299851&r1=299850&r2=299851&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>> (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp Mon
>> Apr 10 11:55:57 2017
>> @@ -807,6 +807,7 @@ Value *InstCombiner::FoldAndOfICmps(ICmp
>> }
>> }
>>
>> + // FIXME: The code below is duplicated in FoldOrOfICmps.
>> // From here on, we only handle:
>> // (icmp1 A, C1) & (icmp2 A, C2) --> something simpler.
>> if (Val != Val2)
>> @@ -825,11 +826,14 @@ Value *InstCombiner::FoldAndOfICmps(ICmp
>>
>> // Ensure that the larger constant is on the RHS.
>> bool ShouldSwap;
>> - if (CmpInst::isSigned(PredL) ||
>> - (ICmpInst::isEquality(PredL) && CmpInst::isSigned(PredR)))
>> - ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
>> - else
>> + if (CmpInst::isUnsigned(PredL) || CmpInst::isUnsigned(PredR)) {
>> + // We have an unsigned compare (possibly with an equality compare),
>> so treat
>> + // the constants as unsigned.
>> ShouldSwap = LHSC->getValue().ugt(RHSC->getValue());
>> + } else {
>> + // Equality transforms treat the constants as signed.
>> + ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
>> + }
>>
>> if (ShouldSwap) {
>> std::swap(LHS, RHS);
>> @@ -877,10 +881,6 @@ Value *InstCombiner::FoldAndOfICmps(ICmp
>> case ICmpInst::ICMP_SGT: // (X != 13 & X s> 15) -> X s> 15
>> return RHS;
>> case ICmpInst::ICMP_NE:
>> - // Special case to get the ordering right when the values wrap
>> around
>> - // zero.
>> - if (LHSC->getValue() == 0 && RHSC->getValue().isAllOnesValue())
>> - std::swap(LHSC, RHSC);
>> if (LHSC == SubOne(RHSC)) { // (X != 13 & X != 14) -> X-13 >u 1
>> Constant *AddC = ConstantExpr::getNeg(LHSC);
>> Value *Add = Builder->CreateAdd(Val, AddC, Val->getName() +
>> ".off");
>> @@ -1727,6 +1727,7 @@ Value *InstCombiner::FoldOrOfICmps(ICmpI
>> return Builder->CreateICmpULE(Val, LHSC);
>> }
>>
>> + // FIXME: The code below is duplicated in FoldAndOfICmps.
>> // From here on, we only handle:
>> // (icmp1 A, C1) | (icmp2 A, C2) --> something simpler.
>> if (Val != Val2)
>> @@ -1745,11 +1746,14 @@ Value *InstCombiner::FoldOrOfICmps(ICmpI
>>
>> // Ensure that the larger constant is on the RHS.
>> bool ShouldSwap;
>> - if (CmpInst::isSigned(PredL) ||
>> - (ICmpInst::isEquality(PredL) && CmpInst::isSigned(PredR)))
>> - ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
>> - else
>> + if (CmpInst::isUnsigned(PredL) || CmpInst::isUnsigned(PredR)) {
>> + // We have an unsigned compare (possibly with an equality compare),
>> so treat
>> + // the constants as unsigned.
>> ShouldSwap = LHSC->getValue().ugt(RHSC->getValue());
>> + } else {
>> + // Equality transforms treat the constants as signed.
>> + ShouldSwap = LHSC->getValue().sgt(RHSC->getValue());
>> + }
>>
>> if (ShouldSwap) {
>> std::swap(LHS, RHS);
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/or.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> Transforms/InstCombine/or.ll?rev=299851&r1=299850&r2=299851&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/test/Transforms/InstCombine/or.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/or.ll Mon Apr 10 11:55:57 2017
>> @@ -223,10 +223,9 @@ define i1 @test19(i32 %A) {
>>
>> define i1 @or_icmps_eq_diff1(i32 %x) {
>> ; CHECK-LABEL: @or_icmps_eq_diff1(
>> -; CHECK-NEXT: [[CMP1:%.*]] = icmp eq i32 %x, -1
>> -; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i32 %x, 0
>> -; CHECK-NEXT: [[LOGIC:%.*]] = or i1 [[CMP1]], [[CMP2]]
>> -; CHECK-NEXT: ret i1 [[LOGIC]]
>> +; CHECK-NEXT: [[X_OFF:%.*]] = add i32 %x, 1
>> +; CHECK-NEXT: [[TMP1:%.*]] = icmp ult i32 [[X_OFF]], 2
>> +; CHECK-NEXT: ret i1 [[TMP1]]
>> ;
>> %cmp1 = icmp eq i32 %x, -1
>> %cmp2 = icmp eq i32 %x, 0
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170413/4c82fa77/attachment.html>
More information about the llvm-commits
mailing list