[PATCH] D155307: [InstCombine] Allow KnownBits to be propagated

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:925
+          if (DemandedMaskLHS.isSubsetOf(LHSKnown.Zero | LHSKnown.One))
             replaceOperand(*I, 0, Constant::getIntegerValue(VTy, LHSKnown.One));
 
----------------
nikic wrote:
> You still need to return here. I think the only thing that needs to change is the incorrect re-declaration of the LHSKnown / RHSKnown variables.
Unfortunately that's not the case. If we return then the instruction is not simplified. 

The problem is that the instruction 

%or.i = call i32 @llvm.fshl.i32(i32 0, i32 0, i32 16)

keeps being checks (infinite loop) and both arguments replaced by a zero. We need to replace this instruction by a zero. That only happens, not when we return, but when we break and go all the way to the block at the end of the function that does the whole instruction simplication:
```
  // If the client is only demanding bits that we know, return the known
  // constant.
  if (DemandedMask.isSubsetOf(Known.Zero|Known.One))
    return Constant::getIntegerValue(VTy, Known.One);
  return nullptr;
```

Do you see any other way of doing this? We could copy this block into the case statement, but I don't think that makes much sense tbh.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:927
 
-          KnownBits RHSKnown = computeKnownBits(I->getOperand(1), Depth + 1, I);
-          if (DemandedMaskRHS.isSubsetOf(RHSKnown.Zero | RHSKnown.One)) {
+          RHSKnown = computeKnownBits(I->getOperand(1), Depth + 1, I);
+          if (DemandedMaskRHS.isSubsetOf(RHSKnown.Zero | RHSKnown.One))
----------------
goldstein.w.n wrote:
> Should the RHS simplification be in an else statement now?
I was thinking I could do both simplifications if necessary in the same loop thereby avoiding the need for an else statement.


================
Comment at: llvm/test/Transforms/InstCombine/2023-07-13-arm-infiniteloop.ll:4
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv4t-unknown-linux-gnueabi"
+
----------------
nikic wrote:
> Don't specify data layout or triple.
Sure - although I am curious what's the policy here, before submitting this, I did confirm other tests in InstCombine do sometimes specify a triple.


================
Comment at: llvm/test/Transforms/InstCombine/2023-07-13-arm-infiniteloop.ll:62
+  ret void
+}
+
----------------
nikic wrote:
> Please minimize the test -- I don't see how this second function could possibly be related for example.
I did run llvm-reduce... I wonder if llvm-reduce is not removing  functions. I will take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155307