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

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
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
----------------
spatel wrote:
> 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?
I will update the comments to 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,
----------------
spatel wrote:
> Is "b * c" -> 6 in this example?
Yeah, thanks for the finding.


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


================
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 }
----------------
spatel wrote:
> I don't understand what happened in these steps. What is the relationship of "*" and "+" to "|" and ","?
That's a little tricky here. The bitset means and/multiplication chain. So when we do a & b, the bitset should be 1 | 2 to set a and b 's corresponding bits.
"," actually the same to "+", I just want to show the pattern means LHS.
How about I write the comments like:

```
{0b1111, 0b1001, 0b0010 , 0b1101} * {0b0001, 0b0101}
-->
(0b1111 + 0b1001 + 0b0010 + 0b1101) * (0b0001 + 0b0101)
-->
(0b1111 + 0b1001 + 0b0010 + 0b1101) * 0b0001+ (0b1111 + 0b1001 + 0b0010 + 0b1101) * 0b0101
-->
(0b1111 | 0b0001) + (0b1001| 0b0001) + (0b0010 | 0b0001) + (0b1101 | 0b0001) + (0b1111 | 0b0101) + (0b1001| 0b0101) + (0b0010 + 0b0101) + (0b1101 + 0b0101)
--> 
(0b1111 + 0b1001 + 0b0010 + 0b1101 + 0b1111 + 0b1101 + 0b0111 + 0b1101
-->
0b1001 + 0b0010 + 0b1101 + 0b0111
-->
{0b1001, 0b0010, 0b1101, 0b0111}
-->
a * d + b + a * c * d + a * b * c
```


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/LogicalExpr.h:43-44
+public:
+  static const uint64_t ExprAllOne = 0x8000000000000000;
+  static const uint64_t ExprZero = 0x4000000000000000;
+
----------------
spatel wrote:
> 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?
Yeah, you are right. We need an assertion here to make sure the max leaf number is less than 62. I will update it later.


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

https://reviews.llvm.org/D142803