[PATCH] D114666: [InstSimplify] Simplify bool icmp with not in LHS

Thu Dec 9 08:36:15 PST 2021

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - thanks for working through the feedback!

================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2728
+      if (Value *X = ExtractNotLHS(LHS))
+        return X;
+
----------------
hasyimibhar wrote:
> 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`.
It's not clear to me what tests the pre-commit / Phabricator hooks are running currently...

But you should always run the regression tests locally to save a lot of suffering:
https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git

Note that "$ ninja check-llvm-transforms" is a subset of the more general "$ ninja check" command" - best to run the entire set of tests, or you'll probably break a lot of bots (and get a lot of fail mails) if any of those tests break locally.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114666/new/

https://reviews.llvm.org/D114666