[PATCH] D114666: [InstSimplify] Simplify bool icmp with not in LHS
Hasyimi Bahrudin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 9 08:07:15 PST 2021
hasyimibhar added inline comments.
================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2728
+ if (Value *X = ExtractNotLHS(LHS))
+ return X;
+
----------------
spatel wrote:
> hasyimibhar wrote:
> > @spatel I missed adding a `break;` here (and below), and I noticed that this is the reason why `check-polly` is failing. Do you know what kind of tests I could add to `icmp-not-bool-contant.ll` to catch this mistake? If it weren't for the failing `check-polly` test, I would have missed this.
> >
> >
> > ```
> > --
> > /Users/hasyimibahrudin/workspace/llvm/llvm-project/polly/test/Support/dumpmodule.ll:78:15: error: AFTEREARLY: expected string not found in input
> > ; AFTEREARLY: polly.split_new_and_old:
> > ^
> > /Users/hasyimibahrudin/workspace/llvm/llvm-project/build/tools/polly/test/Support/Output/dumpmodule.ll.tmp-npm-after-early.ll:4:30: note: scanning from here
> > define internal void @callee(i32 %n, double* noalias nonnull %A, i32 %i) #0 {
> > ^
> > /Users/hasyimibahrudin/workspace/llvm/llvm-project/build/tools/polly/test/Support/Output/dumpmodule.ll.tmp-npm-after-early.ll:9:18: note: possible intended match here
> > body: ; preds = %entry.split, %body
> > ^
> >
> > Input file: /Users/hasyimibahrudin/workspace/llvm/llvm-project/build/tools/polly/test/Support/Output/dumpmodule.ll.tmp-npm-after-early.ll
> > Check file: /Users/hasyimibahrudin/workspace/llvm/llvm-project/polly/test/Support/dumpmodule.ll
> >
> > -dump-input=help explains the following input dump.
> >
> > Input was:
> > <<<<<<
> > 1: ; ModuleID = '<stdin>'
> > 2: source_filename = "<stdin>"
> > 3:
> > 4: define internal void @callee(i32 %n, double* noalias nonnull %A, i32 %i) #0 {
> > check:78'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
> > 5: entry.split:
> > check:78'0 ~~~~~~~~~~~~~
> > 6: %j.cmp1 = icmp sgt i32 %n, 0
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 7: br i1 %j.cmp1, label %body, label %return
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 8:
> > check:78'0 ~
> > 9: body: ; preds = %entry.split, %body
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > check:78'1 ? possible intended match
> > 10: %j2 = phi i32 [ %j.inc, %body ], [ 0, %entry.split ]
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 11: %idx = add i32 %j2, %i
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~
> > 12: %0 = sext i32 %idx to i64
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 13: %arrayidx = getelementptr inbounds double, double* %A, i64 %0
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 14: store double 4.200000e+01, double* %arrayidx, align 8
> > check:78'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > .
> > .
> > .
> > >>>>>>
> > --
> > ```
> >
> >
> > ```
> > --
> > /Users/hasyimibahrudin/workspace/llvm/llvm-project/polly/test/Support/pipelineposition.ll:87:18: error: INLINED2-NEXT: expected string not found in input
> > ; INLINED2-NEXT: [n] -> { Stmt_polly_loop_header_i_us_us[i0, i1] -> [i0, 1, i1] };
> > ^
> > <stdin>:59:13: note: scanning from here
> > Schedule :=
> > ^
> > <stdin>:60:2: note: possible intended match here
> > [n] -> { Stmt_body_i_us[i0, i1] -> [i0, i1] };
> > ^
> >
> > Input file: <stdin>
> > Check file: /Users/hasyimibahrudin/workspace/llvm/llvm-project/polly/test/Support/pipelineposition.ll
> >
> > -dump-input=help explains the following input dump.
> >
> > Input was:
> > <<<<<<
> > .
> > .
> > .
> > 54: n/a
> > 55: Statements {
> > 56: Stmt_body_i_us
> > 57: Domain :=
> > 58: [n] -> { Stmt_body_i_us[i0, i1] : 0 <= i0 < n and 0 <= i1 < n };
> > 59: Schedule :=
> > next:87'0 X error: no match found
> > 60: [n] -> { Stmt_body_i_us[i0, i1] -> [i0, i1] };
> > next:87'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > next:87'1 ? possible intended match
> > 61: MustWriteAccess := [Reduction Type: NONE] [Scalar: 0]
> > next:87'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 62: [n] -> { Stmt_body_i_us[i0, i1] -> MemRef_A[i0 + i1] };
> > next:87'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 63: }
> > next:87'0 ~~~
> > >>>>>>
> >
> > --
> > ```
> I don't know how that mistake could be missed.
>
> 1. If I compile the earlier draft of the patch locally, I see compiler warnings:
>
> ```
> /llvm-project/llvm/lib/Analysis/InstructionSimplify.cpp:2730:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
> case CmpInst::ICMP_ULT: // X <u 0 -> false
>
> ```
>
> 2. If I run the regression tests, there were test failures:
>
> ```
> % ninja check-llvm-transforms
> ...
> Failed Tests (3):
> LLVM :: Transforms/InstSimplify/icmp-bool-constant.ll
> LLVM :: Transforms/InstSimplify/select-inseltpoison.ll
> LLVM :: Transforms/InstSimplify/select.ll
>
> ```
>
> If the tests without a 'not' instruction did not exist already, then we should have added them as part of this patch or the preliminary patch that added tests. But since they already exist, I don't think it is worth duplicating them.
You are right. If I remove the `break`, I can see lots of tests failing with `check-llvm-transforms` (I didn't know about this test). I probably missed this because I was looking at the failing tests on harbormaster, not locally. So I could have missed the other failing tests and only noticed the most recent failed tests, which was `check-polly`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114666/new/
https://reviews.llvm.org/D114666
More information about the llvm-commits
mailing list