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

Fri Feb 17 12:17:33 PST 2023

spatel added a comment.

In D142803#4134002 <https://reviews.llvm.org/D142803#4134002>, @bcl5980 wrote:

> @spatel , do you think this change can split into a individual pass? It looks the sequence of instruction iterator is reversed but I prefer to call the complex logic combine based on normal sequence.
> But I'm not sure if this code is worth to add a new pass. If it is worth, where should this pass been inserted?

It would probably be better for all of the transforms if we update AggressiveInstCombine to use a worklist instead of a simple iterator. VectorCombine was updated like that not too long ago.
But we can defer that to a follow-up if there's no concern about correctness.

I was curious if enabling this for all logic ops would cause any compile-time regressions, but it seems like it has almost no cost:
https://llvm-compile-time-tracker.com/compare.php?from=0e90cd7551f2d0b151f7406e8f3848ec54e650bf&to=ae505cb2a674ac4c240c94a74fc04ee274321697&stat=instructions:u

================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:9-15
+// For the logical expression represent by mask, "*"/"&" is actually or the
+// mask. For example, pattern "a & b" can be represented by logical expression
+// "01 * 10". And the result for "a & b" can be represented by logical
+// expression "11". So the operation "*"/"&" between two basic logical
+// expressions(no "+"/"^", only and chain) is actually or their masks. There is
+// one exception, if one of the operand is constant 0, the mask also represent
+// as 0. We should force the result to 0 not or the masks.
----------------
This is difficult to parse. We must differentiate the logical ops "or" and "and" from the English words.
Header comments should use "///" to auto-generate doxygen.

See if this is a correct edit (adjust to fit 80-columns as necessary):

```

/// For a logical expression represented by bitmasks, the "and" logic operator
/// represented by "&" is translated to "*" and is then evaluated as the "or" of 
/// the bitmasks. For example, pattern "a & b" is represented by the logical 
/// expression "01 * 10", and the expression is reduced to "11". So the 
/// operation "&" between two logical expressions (not "xor", only "and" chain)
/// is actually bitwise "or" of the masks. There is one exception: if one of the 
/// operands is constant 0, the entire mask represents 0. We do not "or" the 
/// masks in that case.

```

================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:17-22
+// The calculation of pattern with "+"/"^" follow one rule, if their are the
+// same masks, we can remove both of them. Like a ^ a, the logical expression is
+// 1 + 1. We can eliminate them from the logical expression then the expression
+// is empty that means it is constant zero.
+//
+// And we can use commutative, associative and distributive laws to caculate. A
----------------
```
/// The evaluation of a pattern for bitwise "xor" is represented by a "+" math operator. 
/// But it also has one exception to normal math rules: if two masks are identical, we 
/// remove them. For example with "a ^ a", the logical expression is "1 + 1". We eliminate 
/// them from the logical expression rather than "or" the bits.
///
/// We use commutative, associative, and distributive laws of arithmetic multiplication
/// and addition to reduce the expression.
```

================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:32
+// Expression after distributive laws:
+//      011 + 111 + 011 + 001 + 100 + 111 + 001
+// Eliminate same masks
----------------
How we got to this is still not obvious. Add at least one intermediate line between these two.
IIUC, we are evaluating "*" before "+" regardless of the parentheses?

================
Comment at: llvm/include/llvm/Analysis/LogicalExpr.h:85
+          continue;
+        uint64_t NewMask = LHSMask | RHSMask;
+        // a & 1 -> a
----------------
It took me a long time to realize that "multiplication" in this expression is really just "bitwise-or". This could use a comment to make it clearer.

================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:9
+//
+// This file help to find the simplest expression for a complex logic
+// operation chain. We canonicalize all other ops to and/xor.
----------------
help to find -> attempts to find

================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:20
+//     a & b ^ c & a ^ b --> {3, 5, 2}
+// Every mask is an and chain. The set of masks is a xor chain.
+// Based on boolean ring, We can treat & as ring multiplication and ^ as ring
----------------
Use quotes around "and" / "xor" / "or" if we are referring to a logic operation.

================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:22
+// Based on boolean ring, We can treat & as ring multiplication and ^ as ring
+// addition. After that, any logic value can be represented by a unsigned set.
+// For example:
----------------
by a unsigned set -> as a chain of bitsets ?

================
Comment at: llvm/lib/Analysis/ComplexLogicCombine.cpp:25-26
+//     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. For now,
+// we only simplify the code when the expression is leaf or null.
+//
----------------
Delete the last line - that's an implementation detail that could change in the future.

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

https://reviews.llvm.org/D142803