[PATCH] Fixing inst-combine not to drops nsw when combining adds into mul (PR19263)
Jingyue Wu
jingyue at google.com
Tue Jun 17 22:16:34 PDT 2014
================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:481
@@ +480,3 @@
+ // Does "(X op Y) op' Z" always equal "(X op' Z) op (Y op' Z)"?
+ if (RightDistributesOverLeft(TopLevelOpcode, InnerOpcode))
+ // Does the instruction have the form "(A op' B) op (C op' B)" or, in the
----------------
Do we still need to try right distribution if left distribution already succeeds?
================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:494
@@ +493,3 @@
+ if (!V && LHS->hasOneUse() && RHS->hasOneUse())
+ V = Builder->CreateBinOp(TopLevelOpcode, A, C, LHS->getName());
+ if (V) {
----------------
A question: any chance to preserve NSW/NUW for TopLevelOpcode?
================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:508
@@ +507,3 @@
+ bool HasNSW = I.hasNoSignedWrap();
+ BinaryOperator *Op0 = dyn_cast<BinaryOperator>(LHS);
+ BinaryOperator *Op1 = dyn_cast<BinaryOperator>(RHS);
----------------
I think it's better to write:
```
if (BinaryOperator *Op0 = dyn_cast<BinaryOperator>(LHS)) {
if (isa<OverflowingBinaryOperator>(Op0)) {
HasNSW &= Op0->hasNoSignedWrap();
}
```
================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:515
@@ +514,3 @@
+
+ cast<BinaryOperator>(SimplifiedInst)->setHasNoSignedWrap(HasNSW);
+ }
----------------
I may be over-concerned, but is this cast dangerous? CreateBinOp returns a ConstantExpr if its operands are both Constant. Note that ConstantInt op ConstantInt is not always foldable, e.g., ptrtoint (a global variable) + 5.
================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:543
@@ +542,3 @@
+
+ // The instruction has the form "(A op' B) op (A)". Try to factorize common
+ // term.
----------------
`(A op' B) op (C)`?
================
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:
> 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
Good point. I agree
```
try(a, b, c, d);
if (op' is commutative) {
try(a, b, d, c);
}
```
is enough.
http://reviews.llvm.org/D3799
More information about the llvm-commits
mailing list