[llvm] r299851 - [InstCombine] fix matching of or-of-icmps constants (PR32524)
NAKAMURA Takumi via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 12 19:27:15 PDT 2017
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/3beea4a4/attachment.html>
More information about the llvm-commits
mailing list