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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I'm still trying to understand how this works, so I only looked at the high-level comments to start.

Is it possible to convert existing logic tests in InstCombien/InstSimplify to use branches, then run those tests with "opt -aggressive-instcombine" and verify that the results are correct?



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/ComplexLogicalOpsCombine.cpp:14
+//    c ? a : b --> (c & a) ^ ((c ^ true) & b)
+// We use a unsigned set to represent the expression. Every value that is not
+// comes from logic operation should be the leaf node. Leaf node is 1 bit in
----------------
I don't understand the term "unsigned set" in this context. Could this be "bit set"? But this is also what you are calling a "mask" later in this description and in the code itself?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/ComplexLogicalOpsCombine.cpp:15
+// We use a unsigned set to represent the expression. Every value that is not
+// comes from logic operation should be the leaf node. Leaf node is 1 bit in
+// the unsigned value. For example, we have source a, b, c. The mask for a is
----------------
Word choice:
"Every value that is not comes from logic operation should be the leaf node." ->
"Any value that is not a logic operation is a leaf node." ?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/ComplexLogicalOpsCombine.cpp:21
+//     a & b ^ c & a ^ b --> {3, 5, 2}
+// Every unsigned value is an and chain. The unsigned set is a xor chain.
+// Based on boolean ring, We can treat & as ring multiplication and ^ as ring
----------------
Now it says "unsigned value", but didn't we just define that as "mask"?
Can we replace "unsigned set" with "set of masks"?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/ComplexLogicalOpsCombine.cpp:25
+// For example:
+//     r1 = (a | b) & c -> r1 = (a * b * c) + (a * c) + (b * c) -> {7, 5, 3}
+// Final we need to rebuild the simplest pattern from the expression. For now,
----------------
Is "b * c" -> 6 in this example?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/LogicalExpr.h:10
+// A example for the LogicalExpr caculation:
+// There are for sources a, b, c, d, the mask for them is 1, 2, 4, 8
+// LHS is (a * b * c * d + a * d + b + a * c * d), RHS is (a + a * c).
----------------
Is it correct to write it like this:
For source values {a,b,c,d}, we can represent them as a bitmask with 'a' as the least-significant-bit: {dcba}.
?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/LogicalExpr.h:13
+// Use bit mask to represent the expression:
+// {0b1111, 0b1001, 0b0010 , 0b1101} * {0b0001, 0b0101}
+// -->
----------------
Does the multiplication between LHS and RHS mean the top-level logic operation in this example is an "and"?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/LogicalExpr.h:15
+// -->
+// {0b1111 | 0b0001, 0b1001| 0b0001, 0b0010 | 0b0001, 0b1101 | 0b0001 } +
+// {0b1111 | 0b0101, 0b1001| 0b0101, 0b0010 | 0b0101, 0b1101 | 0b0101 }
----------------
I don't understand what happened in these steps. What is the relationship of "*" and "+" to "|" and ","?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/LogicalExpr.h:43-44
+public:
+  static const uint64_t ExprAllOne = 0x8000000000000000;
+  static const uint64_t ExprZero = 0x4000000000000000;
+
----------------
If we are using uin64_t as the basic "mask" of values and we have magic constants for the 2 high-bits, does it mean that the "MaxLogicOpLeafsToScan" must be less than 62? Is that enforced with assertions or other logic?


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

https://reviews.llvm.org/D142803