[PATCH] D49179: [InstCombine] Fold x & (-1 >> y) == x to x u<= (-1 >> y)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I'm not too worried about the missing fold in foldLogOpOfMaskedICmps(). That's a complex mess, so not surprising that it has logic holes. I've wondered if we could rewrite that using ranges to simplify that code.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4447-4449
+static Value *
+canonicalizeUnsignedTruncationCheckLikeICmps(ICmpInst &I,
+                                             InstCombiner::BuilderTy &Builder) {
----------------
We're also missing signed folds like:

```
Pre: isPowerOf2(C1+1)
%masked = and i32 %arg, C1
%truncheck = icmp sge i32 %masked, %arg
  =>
%truncheck = icmp sle i32 %arg, C1

```
https://rise4fun.com/Alive/K8H

...so I'd give this function a more general name: foldICmpWithMaskedVal?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4452-4458
+  if (!match(&I, m_c_ICmp(SrcPred,
+                          m_c_And(m_CombineAnd(m_CombineOr(m_LShr(m_AllOnes(),
+                                                                  m_Value()),
+                                                           m_LowBitMask()),
+                                               m_Value(M)),
+                                  m_Value(X)),
+                          m_Deferred(X))))
----------------
Untested, but this could be less indent-crazy if we give some of it a local name like:

  auto m_Mask = m_CombineOr(m_LShr(m_AllOnes(), m_Value()), m_LowBitMask());
  if (!match(&I, m_c_ICmp(SrcPred,
                          m_c_And(m_CombineAnd(m_Mask, m_Value(M)), m_Value(X)),
                          m_Deferred(X))))



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4462
+  ICmpInst::Predicate DstPred;
+  switch (SrcPred) {
+  case ICmpInst::Predicate::ICMP_EQ:
----------------
I think you're planning to extend this soon, but it's best to leave a TODO comment with an explanation just in case that gets delayed.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4659-4660
 
   if (Instruction *Res = foldICmpBinOp(I))
     return Res;
 
----------------
There's no logic to fold ordering within visitICmpInst()...the new function could be called from within foldICmpBinOp() rather than the top-level?


Repository:
  rL LLVM

https://reviews.llvm.org/D49179