[PATCH] Fixing inst-combine not to drops nsw when combining adds into mul (PR19263)

[Dinesh Dwivedi]

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:466
@@ +465,3 @@
+      bool InnerCommutative = Instruction::isCommutative(LHSOpcode);
+      // Does "X op' (Y op Z)" always equal "(X op' Y) op (X op' Z)"?
+      if (LeftDistributesOverRight(LHSOpcode, TopLevelOpcode))
----------------
Jingyue Wu wrote:
> Dinesh Dwivedi wrote:
> > Jingyue Wu wrote:
> > > I feel the logic of this IF and next IF (if (RightDistributesOverLeft) a bit difficult to follow. There is also some copy and paste. What about extracting the check on InnerCommutative out? Something like:
> > > ```
> > > try(a, b, c, d);
> > > if (op' is commutative) {
> > >   try(b, a, c, d);
> > >   try(a, b, d, c);
> > >   try(b, a, c, d);
> > > }
> > > ```
> > > By doing this, inside each "try", we don't need to think about whether op' is commutative. 
> > > 
> > > What do you think?
> > Updated patch to handle this. We can not just depend on isCommutative as
> > there are oparation which are not commutative but still can be distributed e.g.
> > "(X + Y) / Z = X/Z + Y/Z"
> > 
> > We do not handle this now but we might want to do it in future.
> I think the first "try" in my pseudo-code handle this case. Right? 
> 
> While "try" doesn't swap a and b or c and d, it considers both LeftDistributesOverRight and RightDistributesOverLeft. Therefore, it is able to convert "a / b + c / d" to "(a + c) / d". 
Yes, if we considers both LeftDistributesOverRight and RightDistributesOverLeft, it handles all cases. 
But then I think current code is handling all cases more efficiently. 

If left distributes over right and inner op is commutative, then right will also distribute over left 
too and vice versa. So Checking following 2 combinations if left distributes over right
  try(a, b, c, d);
  if (op' is commutative) {
    try(a, b, d, c);
  }
and following 2 combination if right distributes ove left should be sufficient.
  try(a, b, c, d);
  if (op' is commutative) {
    try(a, b, d, c);
  }

Because for inner commutative operation

if try(a, b, c, d) fails for LeftDistributesOverRight then try(b, a, d, c) will fail for RightDistributesOverLeft
if try(a, b, c, d) fails for RightDistributesOverLeft then try(b, a, d, c) will fail for LeftDistributesOverRight

similarly

if try(b, a, c, d) fails for LeftDistributesOverRight then try(a, b, d, c) will fail for RightDistributesOverLeft
if try(a, b, c, d) fails for RightDistributesOverLeft then try(a, b, d, c) will fail for LeftDistributesOverRight

Am I missing something?

http://reviews.llvm.org/D3799