[PATCH] D151660: [InstCombine] (icmp eq A, -1) & (icmp eq B, -1) --> (icmp eq (A&B), -1)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/pr62311.ll:75
+  ret i1 %0
+}
----------------
hnrklssn wrote:
> goldstein.w.n wrote:
> > This single test isn't really sufficient.
> > 
> > Can you add a few more simple tests covering the following cases:
> > 
> > 1) (x == -1 && y == -1)
> > 2) (x != -1) || (y != -1)
> > 3) (x == -1) || (y == -1) -- failure case
> > 4) (x == -1) && (y != -1) -- failure case
> > 
> > maybe a few more failure cases using other constants / mismatched constants.
> > Something in the style of `icmp-add.ll` should be fine.
> >  
> > Can you also split the patch with the tests so we can more easily see the diff generated by this patch.
> > 
> > I.e
> > 
> > Patch1: Test Cases
> > Patch2: Implementation
> > 
> > Then match patch2 the parent of patch1.
> For the success cases, could you also add a second run line to show that this transformation is enough to do the vectorisation asked for in https://github.com/llvm/llvm-project/issues/62311?
> Something like:
> ```
> ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
> ; RUN: opt < %s -passes=instcombine,simplify-cfg,<some-vectoriser-pass> -S | FileCheck --check-prefix INTEGRATED %s
> ```
To check the end-to-end result it would probably be better to add a test in `PhaseOrdering` that just runs `-passes='default<O2>'` (or something) rather than having the InstCombine tests depend on other passes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151660