[PATCH] D67849: [InstCombine] (a+b) < a && (a+b) != 0 -> (0-b) < a iff a/b != 0 (PR43259)

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1064
 
+  auto IsKnownNonZero = [&](Value *V) {
+    return isKnownNonZero(V, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT);
----------------
spatel wrote:
> xbolva00 wrote:
> > lebedev.ri wrote:
> > > xbolva00 wrote:
> > > > lebedev.ri wrote:
> > > > > xbolva00 wrote:
> > > > > > I think we dont need this lambda.. Just inline it.
> > > > > I don't see why spelling all that each time is better.
> > > > So just add a new overload which takes
> > > > 
> > > > isKnownNonZero(V, Q)?
> > > ValueTracking.h (that defines internal `Query` struct) does not
> > > include InstructionSimplify (that defines `SimplifyQuery` struct).
> > > Going in either direction does not seem optimal, and seems out of the scope of the patch.
> > > 
> > > This isn't C, and in C++ i suppose using helpers if they improve code readability is not frowned upon..
> > Thanks for info - just wanted to say: If we can add a overload, we should.. (instead of using lambda).
> > 
> > For me, the 'isKnownNonZero(V, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT);' is fine.
> > 
> > I am personally not very happy to look at the code where there are so many lambdas in one function (what may happen here in the future). I prefer static helper functions..
> > 
> > But this is just my opinion, but I would like to hear opinions of @spatel / @efriedma too.
> > 
> > 
> > 
> > 
> I agree that an overload of some kind would be nicer, but I also agree that it's not necessary to impede this patch with that requirement because it's not a straightforward translation (Query is not the same as SimplifyQuery). 
> 
> If we add a proper overload/adapter, then we can reduce the code duplication here and within InstSimplify itself?
> 
> It's a matter of taste whether the lambda makes the code more or less readable. I'm ok with it.
>> If we add a proper overload/adapter, then we can reduce the code duplication here and within InstSimplify itself?

Yeah, and same for 'isKnownToBeAPowerOfTwo'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67849