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

[Shivam Gupta via Phabricator via llvm-commits]

xgupta marked 3 inline comments as done.
xgupta added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/pr62311.ll:75
+  ret i1 %0
+}
----------------
fhahn wrote:
> 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
Thanks, Updated the test case RUN line here - https://reviews.llvm.org/D151694. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151660