[PATCH] D150360: [InstCombine] Optimize compares with multiple selects as operands

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:6597
+        match(Op1, m_Select(m_Specific(Cond), m_Value(C), m_Value(D))) &&
+        Op0->hasOneUse() && Op1->hasOneUse()) {
+      // Check whether comparison of TrueValues can be simplified
----------------
There are two ways the one-use checks could be relaxed:
1. It should be fine to only require one-use on one of the selects, as we'll preserve the number of instructions in that case. I'm not entirely clear on profitability for that case -- I guess the fact that we're converting an arbitrary select into a logical and/or may make it generally beneficial?
2. If both comparisons fold, we don't care about uses at all, as the result is a constant.

I personally think it would be okay to start with the code you have, but in that case we should at least add a TODO to note the possibility.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-with-selects.ll:4
+
+define i1 @foo(i32 %param, i1 %cond) {
+; CHECK-LABEL: define i1 @foo
----------------
Could you please give these tests some more meaningful names, for example this could be something like `@both_sides_fold`.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-with-selects.ll:126
+  %cond6 = select i1 %cond, i32 %val2, i32 %param
+  %shl = shl i32 %cond1, 4
+  %cmp = icmp sgt i32 %cond6, %cond1
----------------
For multi-use, it's customary to just do a `call void @use(i32 %cond1)`. This is more obvious than trying to come up with a way to combine the uses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150360