[Reassociate] Better preserve NSW/NUW flags. (PR12985)

[mcrosier]

Summary

This change better preserves the NSW/NUW flags in the reassociation pass.

Part of PR12985.

[majnemer]

This transform is not safe for nsw shl, it will transform %shl = shl nsw i32 %a, 31 into %shl = mul nsw i32 %a, -2147483648

If %a is -1, %shl should hold INT_MIN. Instead, it will transform it into poison.

However, I think it's fine to turn a nuw shl into an nuw mul in all cases.
I think you can also turn an nuw nsw shl into an nuw nsw mul.

test/Transforms/Reassociate/wrap-flags.ll

17–28

This transform isn't sound if %y is INT_MIN, %add would be transformed from something valid into poison.

[mcrosier]

Addressed David's comments.

[majnemer]

lib/Transforms/Scalar/Reassociate.cpp

1039–1045

Shouldn't this be:

if (NSW && NUW)

?

[mcrosier]

lib/Transforms/Scalar/Reassociate.cpp

1039–1045

The logic is the same, but in the case that NSW is false it will avoid the call to the ineffectual setHasNoSignedWrap.

I'll fix this.. thanks!

[mcrosier]

Minor tweak to avoid unnecessary call to setter.

[majnemer]

LGTM with tweaks.

lib/Transforms/Scalar/Reassociate.cpp

1045

This can just be Mul->setHasNoSignedWrap(true);

[mcrosier]

Thanks, David! Committed in r221555.

lib/Transforms/Scalar/Reassociate.cpp

 static BinaryOperator *ConvertShiftToMul(Instruction *Shl) {
   Constant *MulCst = ConstantInt::get(Shl->getType(), 1);
   MulCst = ConstantExpr::getShl(MulCst, cast<Constant>(Shl->getOperand(1)));
 
   BinaryOperator *Mul =
     BinaryOperator::CreateMul(Shl->getOperand(0), MulCst, "", Shl);
   Shl->setOperand(0, UndefValue::get(Shl->getType())); // Drop use of op.
   Mul->takeName(Shl);
+
+  // Everyone now refers to the mul instruction.
   Shl->replaceAllUsesWith(Mul);
   Mul->setDebugLoc(Shl->getDebugLoc());
+
+  // We can safely preserve the nuw flag in all cases.  It's also safe to turn a
+  // nuw nsw shl into a nuw nsw mul.  However, nsw in isolation requires special
+  // handling.
+  bool NSW = cast<BinaryOperator>(Shl)->hasNoSignedWrap();
+  bool NUW = cast<BinaryOperator>(Shl)->hasNoUnsignedWrap();
+  if (NUW && NSW)
+    Mul->setHasNoSignedWrap(NSW);
+  Mul->setHasNoUnsignedWrap(NUW);
   return Mul;
 }
 
 /// FindInOperandList - Scan backwards and forwards among values with the same
 /// rank as element i to see if X exists.  If X does not exist, return i.  This
 /// is useful when scanning for 'x' when we see '-x' because they both get the
 /// same rank.
 static unsigned FindInOperandList(SmallVectorImpl<ValueEntry> &Ops, unsigned i,

test/Transforms/Reassociate/wrap-flags.ll

+; RUN: opt < %s -reassociate -dce -S | FileCheck %s
+; PR12985
+
+; Verify the nsw flags are preserved when converting shl to mul.
+
+; CHECK-LABEL: @shl_to_mul_nsw(
+; CHECK: %mul = mul i32 %i, -2147483648
+; CHECK: %mul2 = add i32 %mul, 1
+define i32 @shl_to_mul_nsw(i32 %i) {
+entry:
+  %mul = shl nsw i32 %i, 31
+  %mul2 = add i32 %mul, 1
+  ret i32 %mul2
+}
+
+; CHECK-LABEL: @shl_to_mul_nuw(
+; CHECK: %mul = mul nuw i32 %i, 4
+; CHECK: %mul2 = add i32 %mul, 1
+define i32 @shl_to_mul_nuw(i32 %i) {
+entry:
+  %mul = shl nuw i32 %i, 2
+  %mul2 = add i32 %mul, 1
+  ret i32 %mul2
+}
+
+; CHECK-LABEL: @shl_to_mul_nuw_nsw(
+; CHECK: %mul = mul nuw nsw i32 %i, 4
+; CHECK: %mul2 = add i32 %mul, 1
+define i32 @shl_to_mul_nuw_nsw(i32 %i) {
+entry:
+  %mul = shl nuw nsw i32 %i, 2
+  %mul2 = add i32 %mul, 1
+  ret i32 %mul2
+}