[PATCH] D139630: [InstCombine] Fold logic-and/logic-or by distributive laws part2

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a subscriber: nikic.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2801
       bool FalseLogicAnd = isa<SelectInst>(FalseVal);
-      if (CondLogicAnd && FalseLogicAnd) {
-        // (A ? B : 0) ? 1 : (A ? D : 0) --> A ? (B ? 1 : D) : 0
+      auto AndFactorization = [&](Value *Common, Value *InnerCond,
+                                  Value *InnerVal,
----------------
bcl5980 wrote:
> spatel wrote:
> > bcl5980 wrote:
> > > spatel wrote:
> > > > Can we add some description of the different possibilities? 
> > > > 
> > > > I know that handling all of the patterns poison-safely is not uniform, but what if we use freeze to make it safe with regular logic ops instead:
> > > > https://alive2.llvm.org/ce/z/xb_moG
> > > > 
> > > > There is an existing code example for a similar transform below this block of code - search for "MayNeedFreeze".
> > > > 
> > > > 
> > > So which way are you prefer now? Not uniform code without freeze instruction or use freeze asap to make code easier?
> > If there are no known regressions, let's go straight to the code with freeze. That should be the ideal form for most of these patterns.
> I try a lot of ways to add freeze into the code today. But it looks it will make the code more complicate. 
> Current code already fix all cases without freeze. What freeze can get benefit is convert logical-and to normal and, logical-or to normal or.
> So the question becomes which pattern is better `freeze + and + or`,  `select + select`? Only if the first pattern is better we need to add freeze I think.
> I'm sorry I haven't find a elegant way to do it. Can you give me some suggestions for that?
I don't have any good suggestions, so I suppose this is ok. 

It might be possible to reduce select in some of these patterns to a real logic (and/or) instruction as a smaller step. 

@nikic or @lebedev.ri  might have other ideas.

The tests looks complete, so presumably the logic is correct - have you verified that everything passes with Alive2?


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

https://reviews.llvm.org/D139630