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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2728
+      if (Value *X = ExtractNotLHS(LHS))
+        return X;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114666