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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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));
 
----------------
pmatos wrote:
> 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.
Okay, I understand the problem now. We are replacing the operand with `Constant::getIntegerValue(VTy, LHSKnown.One)` here, but it may be that the operand //already// has that value, in which case we'll go into an infinite loop.

I think what you need to do in that case is check whether we're actually going to change something:
```
if (DemandedMaskLHS.isSubsetOf(LHSKnown.Zero | LHSKnown.One)) {
  Constant *NewLHS = Constant::getIntegerValue(VTy, LHSKnown.One);
  if (I->getOperand(0) != NewLHS) {
    replaceOperand(*I, 0, NewLHS);
    return &I;
  }
}
```
That should make sure we only report a change if it actually happened.


================
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"
+
----------------
pmatos wrote:
> 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.
If you specify a triple you also need to specify either REQUIRES or move it into a target-specific directory. It's best to omit triple if it's not strictly necessary.


================
Comment at: llvm/test/Transforms/InstCombine/2023-07-13-arm-infiniteloop.ll:62
+  ret void
+}
+
----------------
pmatos wrote:
> 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.
llvm-reduce should be removing functions...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155307