spatel added a comment. A few changes for tests suggested inline.
There might be some generalization of ctpop analysis that we can make as a follow-up patch. For example, I was looking at a "wrong predicate" combination and noticed that we miss possible optimizations like this: https://alive2.llvm.org/ce/z/RRk_d9 ================ Comment at: llvm/test/Transforms/InstCombine/icmp-or.ll:412 + %4 = icmp eq <2 x i32> %0, <i32 0, i32 0> + %5 = or <2 x i1> %4, %3 + ret <2 x i1> %5 ---------------- We can swap the operand order of the "or" for more coverage of the commuted pattern. ================ Comment at: llvm/test/Transforms/InstCombine/icmp-or.ll:444 + %4 = icmp ne i32 %0, 0 + %5 = and i1 %4, %3 + ret i1 %5 ---------------- We are testing the 'and' pattern too now, so it doesn't match the name of the file. I think it would be better to add the new tests next to the changed tests in `ispow2.ll`. ================ Comment at: llvm/test/Transforms/InstCombine/icmp-or.ll:478 + +; negative test - wrong constants + ---------------- Instead of checking the same wrong constant again, it would be better to change to a wrong predicate(s). ================ Comment at: llvm/test/Transforms/InstCombine/icmp-or.ll:382 + ret i1 %5 +} + ---------------- craig.topper wrote: > RKSimon wrote: > > What about if the ctpop has multi uses? > The ctpop isn't being changed. Does it matter if it has more uses? > > What is interesting is if the (icmp eq (ctpop(x)), 1) has another user other > than the or. This transform only replaces the logic op with a cmp, so I think we want to do it even if all of the intermediate values have other uses. Either way, we should have at least one test where there are other uses of the icmps. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122077/new/ https://reviews.llvm.org/D122077 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
