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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

This seems close to ready to me. It would be great if other reviewers have a look too. :)



================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:1
+//===-------- ComplexLogicalOpsCombine.cpp -------------------------------===//
+//
----------------
I think we should remove "Complex" from the name. This can handle all LogicOp combining now (and eventually, it could be part of regular InstCombine).


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:21
+// Every bitset is an "&" chain. The set of bitset is a "^" chain.
+// Based on boolean ring, We can treat "&" as ring multiplication and "^" as
+// ring addition. After that, any logic value can be represented as a chain of
----------------
We -> we


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:25
+//     r1 = (a | b) & c -> r1 = (a * b * c) + (a * c) + (b * c) -> {7, 5, 6}
+// Final we need to rebuild the simplest pattern from the expression.
+//
----------------
Final -> Finally,


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:45
+static cl::opt<unsigned> MaxLogicOpLeafsToScan(
+    "clc-max-logic-leafs", cl::init(8), cl::Hidden,
+    cl::desc("Max leafs of logic ops to scan for complex logical combine."));
----------------
If you agree with the earlier comment about removing "Complex" from the name, then change the "clc" also. It could just be "logic-combine-max-leafs" or something like that.


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:65
+
+  unsigned LeafCnt = llvm::popcount(LeafBits);
+  if (LeafBits == 0 || LeafCnt == 0)
----------------
Should not need "llvm::" specifier here?


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:66
+  unsigned LeafCnt = llvm::popcount(LeafBits);
+  if (LeafBits == 0 || LeafCnt == 0)
+    return;
----------------
The "LeafCnt == 0" is redundant. Move the "LeafBits == 0" check above the popcount for efficiency.


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:76
+  for (unsigned I = 1; I < LeafCnt; I++) {
+    LeafIdx = llvm::countr_zero(LeafBits);
+    printValue(OS, Helper->LeafValues[LeafIdx]);
----------------
Shouldn't need "llvm::" ?


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:135-136
+  // We can only to simpilfy and, or , xor in the binary operator
+  if (BO->getOpcode() != Instruction::And &&
+      BO->getOpcode() != Instruction::Or && BO->getOpcode() != Instruction::Xor)
+    return visitLeafNode(BO, Depth);
----------------
`!BO->isBitwiseLogicOp()`


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:180
+  const LogicalExpr &Expr = Node->getExpr();
+  // Empty when all leaf bits are earsed from the set because a ^ a = 0.
+  if (Expr.size() == 0)
----------------
earsed -> erased


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:196
+
+  // TODO: complex pattern simpilify
+
----------------
What does this TODO mean? Either describe the planned follow-up with more details or remove this line.


================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:203
+  assert(MaxLogicOpLeafsToScan <= 63 &&
+         "Logical leaf node can't larger than 63.");
+  LogicalOpNode *RootNode = getLogicalOpNode(Root);
----------------
"can't be larger"


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:830-831
+static bool foldComplexLogic(LogicalOpsHelper &Helper, Instruction &I) {
+  if (I.getOpcode() == Instruction::And || I.getOpcode() == Instruction::Or ||
+      I.getOpcode() == Instruction::Xor) {
+    Value *NewV = Helper.simplify(&I);
----------------
`I.isBitwiseLogicOp()`


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:854-855
 
+    // For now, we share logical expression on basic block level.
+    LogicalOpsHelper Helper;
+
----------------
What does the "for now" mean? Will it change in the future? What limitation does this imply?


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/complex-logic.ll:12
 
+define i1 @leaf1_and_a_false(i1 %a)  {
+; CHECK-LABEL: @leaf1_and_a_false(
----------------
We should have more than i1 types in these tests if we are handling all bitwise logic now.


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

https://reviews.llvm.org/D142803