[PATCH] D142803: [LogicCombine 1/?] Implement a general way to simplify logical operations.

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added a comment.

In D142803#4147225 <https://reviews.llvm.org/D142803#4147225>, @spatel wrote:

>> I need LeafValues to access value by index. It looks SmallSetVector can't do that.
>
> SmallSetVector allows indexing. This is the patch I tried after applying this patch (I had to fix line endings first), and no tests fail:
>
>   diff --git a/llvm/include/llvm/Analysis/LogicCombine.h b/llvm/include/llvm/Analysis/LogicCombine.h
>   index 3fdcf7998321..56a3d8f36b16 100644
>   --- a/llvm/include/llvm/Analysis/LogicCombine.h
>   +++ b/llvm/include/llvm/Analysis/LogicCombine.h
>   @@ -8,8 +8,7 @@
>   
>    #include "LogicalExpr.h"
>    #include "llvm/ADT/DenseMap.h"
>   -#include "llvm/ADT/SmallPtrSet.h"
>   -#include "llvm/ADT/SmallVector.h"
>   +#include "llvm/ADT/SetVector.h"
>    #include "llvm/ADT/Statistic.h"
>    #include "llvm/IR/InstrTypes.h"
>    #include "llvm/IR/Instruction.h"
>   @@ -50,8 +49,7 @@ private:
>      friend class LogicalOpNode;
>   
>      SmallDenseMap<Value *, LogicalOpNode *, 16> LogicalOpNodes;
>   -  SmallPtrSet<Value *, 8> LeafSet;
>   -  SmallVector<Value *, 8> LeafValues;
>   +  SmallSetVector<Value *, 8> LeafValues;
>   
>      void clear();
>   
>   diff --git a/llvm/lib/Analysis/LogicCombine.cpp b/llvm/lib/Analysis/LogicCombine.cpp
>   index 28d9488cab96..3b410cdacd32 100644
>   --- a/llvm/lib/Analysis/LogicCombine.cpp
>   +++ b/llvm/lib/Analysis/LogicCombine.cpp
>   @@ -101,17 +101,16 @@ void LogicCombiner::clear() {
>      for (auto node : LogicalOpNodes)
>        delete node.second;
>      LogicalOpNodes.clear();
>   -  LeafSet.clear();
>      LeafValues.clear();
>    }
>   
>    LogicalOpNode *LogicCombiner::visitLeafNode(Value *Val, unsigned Depth) {
>      // Depth is 0 means the root is not logical operation. We can't
>      // do anything for that.
>   -  if (Depth == 0 || LeafSet.size() >= MaxLogicOpLeafsToScan)
>   +  if (Depth == 0 || LeafValues.size() >= MaxLogicOpLeafsToScan)
>        return nullptr;
>   
>   -  uint64_t ExprVal = 1ULL << LeafSet.size();
>   +  uint64_t ExprVal = 1ULL << LeafValues.size();
>      // Constant Zero,AllOne are special leaf nodes. They involve
>      // LogicalExpr's calculation so we must detect them at first.
>      if (auto ConstVal = dyn_cast<ConstantInt>(Val)) {
>   @@ -120,9 +119,8 @@ LogicalOpNode *LogicCombiner::visitLeafNode(Value *Val, unsigned Depth) {
>        else if (ConstVal->isAllOnesValue())
>          ExprVal = LogicalExpr::ExprAllOne;
>      }
>   -  if (ExprVal != LogicalExpr::ExprAllOne && ExprVal != 0 &&
>   -      LeafSet.insert(Val).second)
>   -    LeafValues.push_back(Val);
>   +  if (ExprVal != LogicalExpr::ExprAllOne && ExprVal != 0)
>   +    LeafValues.insert(Val);
>      LogicalOpNode *Node = new LogicalOpNode(this, Val, LogicalExpr(ExprVal));
>      LogicalOpNodes[Val] = Node;
>      return Node;

Thanks, I have already update it.


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

https://reviews.llvm.org/D142803