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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LogicCombine.h:22
+private:
+  LogicCombiner *Helper;
+  Value *Val;
----------------
It's weird that LogicalOpNode has a reference back to LogicCombiner. Is this just for printing? Would it be better to pass it to the print method?


================
Comment at: llvm/include/llvm/Analysis/LogicCombine.h:34
+      : Helper(OpsHelper), Val(SrcVal), Expr(SrcExpr) {}
+  ~LogicalOpNode() {}
+
----------------
Why the explicit empty dtor?


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:10
+/// This file defines LogicalExpr, a class that represent a logical value by
+/// a set of bitset.
+///
----------------
bitsets?


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:27
+/// We use commutative, associative, and distributive laws of arithmetic
+/// multiplication and addition to reduce the expression. A example for the
+/// LogicalExpr caculation:
----------------
An example


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:56
+  // TODO: can we use APInt define the mask to enlarge the max leaf number?
+  uint64_t LeafMask;
+
----------------
Am I missing something, or is LeafMask never actually used?


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:58
+
+  inline void updateLeafMask() {
+    LeafMask = 0;
----------------
Unnecessary `inline`?


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:86
+      // a & 0 -> 0
+      if (LHS == 0)
+        continue;
----------------
Hm, can these actually occur? It looks like they should be excluded by a ^ a canonicalization.


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:94
+        uint64_t NewBitSet;
+        // Except the speical case one value "*" -1 is just return itself, the
+        // other "*" operation is actually "|" LHS and RHS 's bitset. For
----------------
spatel wrote:
> spelling: "special"
special


================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:120
+      // a ^ a -> 0
+      if (!AddChain.insert(RHS).second)
+        AddChain.erase(RHS);
----------------
insert returns an iterator you can pass to erase.


================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:16
+// logic operation is a leaf node. Leaf node is 1 bit in the bitset. For
+// example, we have source a, b, c. The bit for a is 1, b is 2 ,c is 4.
+//     a & b & c --> {7}
----------------



================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:17
+// example, we have source a, b, c. The bit for a is 1, b is 2 ,c is 4.
+//     a & b & c --> {7}
+//     a & b ^ c & a --> {3, 5}
----------------
It would be more helpful to write them in binary form here, e.g. `111`.


================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:51
+
+void LogicalOpNode::printValue(raw_ostream &OS, Value *Val) const {
+  if (auto *ConstVal = dyn_cast<Constant>(Val))
----------------
I think you might be looking for `Value->printAsOperand()` here?


================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:77
+    printValue(OS, Helper->LeafValues[LeafIdx]);
+    OS << " * ";
+    LeafBits -= (1ULL << LeafIdx);
----------------
You can use `ListSeparator` and avoid the need to split out the last case.


================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:93
+    printAndChain(OS, *I);
+    OS << " + ";
+  }
----------------
Same here, ListSeparator.


================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:102
+  for (auto node : LogicalOpNodes)
+    delete node.second;
+  LogicalOpNodes.clear();
----------------
Should these be using unique_ptr instead?

Or possibly SpecificBumpPtrAllocator?


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

https://reviews.llvm.org/D142803