[PATCH] D145157: [InstCombine] Implement "A & (~A | B) --> A & B" like transforms for boolean based selects.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

This looks close to me, but see inline comments on the tests, and please pre-commit those with the baseline CHECK lines, so we'll just show test diffs in this patch.



================
Comment at: llvm/test/Transforms/InstCombine/logical-select.ll:1150
+; A & (~A | B) --> A & B
+define i1 @and_commute2(i1 %a, i1 %b) {
+; CHECK-LABEL: @and_commute2(
----------------
The test name is a bit misleading here - it's not a commute variation; it's replacing a bitwise 'or' with a 'logical-or'.


================
Comment at: llvm/test/Transforms/InstCombine/logical-select.ll:1162
+; A & (~A | B) --> A & B
+define <2 x i1> @and_commute2_vec(<2 x i1> %a, <2 x i1> %b) {
+; CHECK-LABEL: @and_commute2_vec(
----------------
Instead of repeating the test with a vector type (which we already verified works in the 2nd test), this test would provide better coverage if it included extra uses of the intermediate logic ops. That shows that the transform is still valid and intentional regardless of other uses.


================
Comment at: llvm/test/Transforms/InstCombine/logical-select.ll:1173
+
+; A & (~A | B) --> A & B
+; As and_commute1 but with %or's operands switched
----------------
It would be better to keep this formula synchronized with the test (and the comment on the next line), so commute: 
"A & (B | ~A)"


================
Comment at: llvm/test/Transforms/InstCombine/logical-select.ll:1188
+
+; A | (~A & B) -> A | B
+define i1 @or_commute1(i1 %a, i1 %b) {
----------------
Same comments for this set of tests as above, but a couple of additional potential tests:
1. Include a poison element in the vector constant to show that works too.
2. Add a negative test where 'not %a' is replaced by an arbitary value to show that the transform only works when we have a specific value match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145157