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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Please rename the test file as a preliminary step, so we will again show diffs in this patch. We also need to add negative tests to show current limitations and also that the combining is not making wrong logic reductions.



================
Comment at: llvm/include/llvm/Analysis/LogicCombine.h:42
+
+class LogicCombineHelper {
+public:
----------------
This isn't really a "helper" - this is the main part of the code. Can we name this "LogicCombiner"?


================
Comment at: llvm/include/llvm/Analysis/LogicCombine.h:53-54
+  SmallDenseMap<Value *, LogicalOpNode *, 16> LogicalOpNodes;
+  SmallPtrSet<Value *, 8> LeafSet;
+  SmallVector<Value *, 8> LeafValues;
+
----------------
I don't think we need "LeafSet". If you make "LeafValues" a `SmallSetVector`, we won't insert duplicate values into the vector. Is that the only job of the LeafSet?


================
Comment at: llvm/lib/Analysis/LogicCombine.cpp:133
+                                              unsigned Depth) {
+  // We can only to simpilfy and, or , xor in the binary operator
+  if (!BO->isBitwiseLogicOp())
----------------
This comment isn't necessary - I think the code is clear enough now.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:828
 
+// Use LogicalOpsHelper to simplify complex logic
+static bool foldComplexLogic(LogicCombineHelper &Helper, Instruction &I) {
----------------
/// Reduce bitwise logic sequences.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:853-859
+    // There are 2 limitations to be trade off here. The higher level the
+    // LogicalOpsHelper create, the more logical node cached, which means it can
+    // save more cpu timing. But it will maintain more leaf nodes. By default
+    // the max of leaf node is 8 , which is not enough for whole function I
+    // guess. We can improve it later. Like split the helper based on types to
+    // make the code more efficient, adjust the default value of max leaf node
+    // number, use APInt to support more bits.
----------------
Adjust wording (make sure I understand it correctly):
    // TODO: Combining at the function-level would allow more 
    // caching of nodes which saves on compile-time, but it may hit the
    // max depth or value limits before finding a solution. We could split
    // the helper based on types to make the code more efficient, adjust 
    // the value of max depth/values, or use APInt to support tracking 
    // more than 63 leaf values.

But I doubt that we have a real problem here for the vast majority of programs? We are tracking up to 8 different leaf values with a depth of 8 logic instructions. Maybe add (currently) negative tests with those large number of instructions to show what is needed to hit the limits?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:878
       MadeChange |= foldSqrt(I, TTI, TLI);
+      MadeChange |= foldComplexLogic(Helper, I);
     }
----------------
Change name: foldComplexLogic -> foldBitwiseLogic

This call should be moved before "foldSqrt()" as the code comment suggests.


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

https://reviews.llvm.org/D142803