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

[Dinesh Dwivedi]

typo :(

================
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))
----------------
Dinesh Dwivedi wrote:
> 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?
Please read last line in previous reply as
if try(b, a, c, d) fails for RightDistributesOverLeft then try(a, b, d, c) will fail for LeftDistributesOverRight

http://reviews.llvm.org/D3799