[PATCH] D142542: [InstSimplify] Simplify icmp between Left-Shifted VScale Calls

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3634
+  ConstantInt *IntLHS, *IntRHS;
+  if (!match(LHS, m_Shl(m_VScale(Q.DL), m_ConstantInt(IntLHS))) ||
+      !match(RHS, m_Shl(m_VScale(Q.DL), m_ConstantInt(IntRHS))) ||
----------------
MattDevereau wrote:
> dmgreen wrote:
> > MattDevereau wrote:
> > > sdesmalen wrote:
> > > > Does it matter that this is value is `vscale` ?
> > > > 
> > > > I would expect that:
> > > > * `(X << C1) < (X << C2) == true` for any value `X` for any `C1 < C2`
> > > > * `(X << C1) <= (X << C2) == true` for any value `X` for any `C1 <= C2`
> > > > * `(X << C1) > (X << C2) == true` for any value `X` for any `C1 > C2`
> > > > * ...
> > > Very true, I think I got a bit too tunnel visioned on the original test case. I will replace the vscale checks with m_Value for LHS and m_Deferred for RHS, and remove any references to vscale in names
> > Make sure you check the value isn't 0, and it can be good to provide alive proofs.
> Noted, thank you
https://alive2.llvm.org/ce/z/UAsDSn This alive test shows the transformation is not valid for the case where value is 0, however
I'm not sure how to write a test to assert the not-zero 0 check here, something simple like
 
```
define i1 @compare_zeroes_foo() {
; CHECK-LABEL: @compare_zeroes_foo(
; CHECK-NEXT:    ret i1 false
;
  %shl1 = shl nuw nsw i64 0, 1
  %shl2 = shl nuw nsw i64 0, 2
  %cmp = icmp ult i64 %shl1, %shl2
  ret i1 %cmp
}
```

Has already optimized away the shifts by the time it reaches my added function `simplifyICmpLShiftedValues`, and when LHS and RHS are both `ConstantInt 0` this will be optimized to `return i1 false` by `ConstantFoldCompareInstOperands` above. I'm not sure if a simple check such as


```
  ConstantInt *IntLHS, *IntRHS;
  Value* V;
  if (!match(LHS, m_Shl(m_Value(V), m_ConstantInt(IntLHS))) ||
      !match(RHS, m_Shl(m_Deferred(V), m_ConstantInt(IntRHS))) ||
      LHS->getType() != RHS->getType())
    return nullptr;

  if(auto ConstantIntV = dyn_cast<ConstantInt>(V)){
    if(ConstantIntV->isZero())
      return nullptr;
  }
...
```
Is sufficient or even necessary here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142542