[PATCH] D145299: [InstCombine] Generate better code for std::bit_ceil

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3225
+      CR = CR.binaryNot();
+      RangeOp = CtlzOp;
+    } else {
----------------
These RangeOp assignments are all dead?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3263
+static Instruction *foldBitCeil(SelectInst &SI,
+                                InstCombiner::BuilderTy &Builder) {
+  Type *SelType = SI.getType();
----------------
`IRBuilderBase &` is preferred.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3270
+  ICmpInst::Predicate Pred;
+  const APInt *Cond1 = nullptr;
+  Value *Cond0, *Ctlz, *CtlzOp;
----------------
Drop nullptr init.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3292
+  // hardware as part of the shift instruction.
+  Value *Neg = Builder.CreateSub(ConstantInt::getNullValue(SelType), Ctlz);
+  Value *Masked =
----------------
`CreateNeg`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3193
+      match(CtlzOp, m_Not(m_Value(X)))) {
+    // We'll stop following the def-use chain when we encounter X.
+  }
----------------
kazu wrote:
> RKSimon wrote:
> > empty clause?
> I would like to find `X` here and leave it `nullptr` if no match occurs.  I don't have any actions when the `if` condition is true.  Should I say something like this?
> 
> ```
>   (void) match(CtlzOp, m_Add(m_Value(X), m_ConstantInt())) ||
>       match(CtlzOp, m_Sub(m_ConstantInt(), m_Value(X))) ||
>       match(CtlzOp, m_Not(m_Value(X)));
> ```
> 
Something that I don't really get is why we have to do this separately. Wouldn't something like this be sufficient?
```
Value *RangeOp = Cond0;
ConstantRange CR = ConstantRange::makeExactICmpRegion(
    CmpInst::getInversePredicate(Pred), *Cond1);

if (CtlzOp != RangeOp) {
  if (match(CtlzOp, m_Add(m_Specific(RangeOp), m_APInt(C)))) {
    CR = CR.add(*C);
  }
  // ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145299