InstCombine: Insert missing canonicalizes

[arsenm]

Summary

minnum/maxnum are supposed to return a canonicalized value.
We were folding cases where one of the operands is constant
without inserting the appropriate canonicalize. Also fix
the LangRef to mention the canonicalize part of the IEEE
definition.

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[arsenm]

ping

[scanon]

IEEE 754 rules are that everything canonicalizes except bitwise operations (copy, abs, negate, copysign) and decimal re-encoding operations (which you don't care about).

[escha]

In D37989#936337, @scanon wrote:

IEEE 754 rules are that everything canonicalizes except bitwise operations (copy, abs, negate, copysign) and decimal re-encoding operations (which you don't care about).

Does this mean that we need to make all other float optimizations in LLVM do the same?

for example, we cannot optimize fmul(x, 1) to x, we must optimize it to fcanonicalize(x), right? thus preventing pretty much all float optimizations, since presumably that will act as a barrier.

[arsenm]

In D37989#936369, @escha wrote:

In D37989#936337, @scanon wrote:

IEEE 754 rules are that everything canonicalizes except bitwise operations (copy, abs, negate, copysign) and decimal re-encoding operations (which you don't care about).

Does this mean that we need to make all other float optimizations in LLVM do the same?

for example, we cannot optimize fmul(x, 1) to x, we must optimize it to fcanonicalize(x), right? thus preventing pretty much all float optimizations, since presumably that will act as a barrier.

Also related is D37999. Is it OK to universally constant fold some subset of canonicalizes?

[scanon]

In D37989#936369, @escha wrote:

In D37989#936337, @scanon wrote:

IEEE 754 rules are that everything canonicalizes except bitwise operations (copy, abs, negate, copysign) and decimal re-encoding operations (which you don't care about).

Does this mean that we need to make all other float optimizations in LLVM do the same?

for example, we cannot optimize fmul(x, 1) to x, we must optimize it to fcanonicalize(x), right? thus preventing pretty much all float optimizations, since presumably that will act as a barrier.

Well, so far we're saved by the fact that there are no non-canonical floats or doubles in IEEE 754 (platforms that treat denorms as non-canonical zeros are already outside IEEE 754, so do whatever you like). Non-canonical encodings *do* exist for float80, but they canonicalize if you look at them funny, so it hasn't been a problem.

If we want to get this really formally right at some point, then yes, fmul(x,1) would become fcanonicalize(x), which would usually be consumed into whatever consumes the result of the multiply, because everything except the aforementioned functions is described in terms of canonicalized inputs.

[escha]

Regardless of semantics, this patch almost surely causes a major problem for us.

I originally added fcanonicalize for the purpose of our numerics folks, who wanted to be able to force canonicalizations in numerics tests and so forth. It was meant to be a "dumb intrinsic" that would just blindly emit a no-op float instruction to force canonicalization of the input.

If we start emitting fcanonicalize all over the place in instcombine, it's going to destroy our codegen quality and significantly increase instruction count.

Do we really need to do this for the sake of float80 or something?

[arsenm]

We also don't have optimizations now to eliminate canonicalizes when they are redundant, but could add them

[escha]

Even if they're not redundant, it's something we still don't want.

For example: imagine a function that takes two input texture reads, takes the max, and stores that. On our arch, that wouldn't canonicalize, so we would be forced to insert an additional arithmetic instruction for every single "max", doubling the amount of math in the function.

docs/LangRef.rst

 type.
 
 Semantics:
 """"""""""
 
 Follows the IEEE-754 semantics for minNum, which also match for libm's
 fmin.
 
-If either operand is a NaN, returns the other non-NaN operand. Returns
-NaN only if both operands are NaN. If the operands compare equal,
-returns a value that compares equal to both operands. This means that
-fmin(+/-0.0, +/-0.0) could return either -0.0 or 0.0.
+If either operand is a NaN, returns the canonicalized other non-NaN
+operand. Returns NaN only if both operands are NaN. If the operands
+compare equal, returns a value that compares equal to both
+operands. This means that fmin(+/-0.0, +/-0.0) could return either
+-0.0 or 0.0.
 
 '``llvm.maxnum.*``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Syntax:
 """""""
 
 This is an overloaded intrinsic. You can use ``llvm.maxnum`` on any

 The arguments and return value are floating point numbers of the same
 type.
 
 Semantics:
 """"""""""
 Follows the IEEE-754 semantics for maxNum, which also match for libm's
 fmax.
 
-If either operand is a NaN, returns the other non-NaN operand. Returns
-NaN only if both operands are NaN. If the operands compare equal,
-returns a value that compares equal to both operands. This means that
-fmax(+/-0.0, +/-0.0) could return either -0.0 or 0.0.
+If either operand is a NaN, returns the canonicalized other non-NaN
+operand. Returns NaN only if both operands are NaN. If the operands
+compare equal, returns a value that compares equal to both
+operands. This means that fmax(+/-0.0, +/-0.0) could return either
+-0.0 or 0.0.
 
 '``llvm.copysign.*``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Syntax:
 """""""
 
 This is an overloaded intrinsic. You can use ``llvm.copysign`` on any

include/llvm/IR/IRBuilder.h

public:
   /// \brief Create a call to the experimental.gc.relocate intrinsics to
   /// project the relocated value of one pointer from the statepoint.
   CallInst *CreateGCRelocate(Instruction *Statepoint,
                              int BaseOffset,
                              int DerivedOffset,
                              Type *ResultType,
                              const Twine &Name = "");
 
+  /// Create a call to intrinsic \p ID with 1 operand which is mangled on the
+  /// first type.
+  CallInst *CreateUnaryIntrinsic(Intrinsic::ID ID, Value *Op,
+                                 const Twine &Name = "");
+
+  CallInst *CreateCanonicalize(Value *Op, const Twine &Name = "") {
+    return CreateUnaryIntrinsic(Intrinsic::canonicalize, Op, Name);
+  }
+
   /// Create a call to intrinsic \p ID with 2 operands which is mangled on the
   /// first type.
   CallInst *CreateBinaryIntrinsic(Intrinsic::ID ID,
                                   Value *LHS, Value *RHS,
                                   const Twine &Name = "");
 
   /// Create call to the minnum intrinsic.
   CallInst *CreateMinNum(Value *LHS, Value *RHS, const Twine &Name = "") {

lib/IR/IRBuilder.cpp

Value *FnGCRelocate =
    Intrinsic::getDeclaration(M, Intrinsic::experimental_gc_relocate, Types);
 
  Value *Args[] = {Statepoint,
                   getInt32(BaseOffset),
                   getInt32(DerivedOffset)};
  return createCallHelper(FnGCRelocate, Args, this, Name);
 }
 
+CallInst *IRBuilderBase::CreateUnaryIntrinsic(Intrinsic::ID ID, Value *Op,
+                                              const Twine &Name) {
+  Module *M = BB->getParent()->getParent();
+  Function *Fn = Intrinsic::getDeclaration(M, ID, { Op->getType() });
+  return createCallHelper(Fn, { Op }, this, Name);
+}
+
 CallInst *IRBuilderBase::CreateBinaryIntrinsic(Intrinsic::ID ID,
                                                Value *LHS, Value *RHS,
                                                const Twine &Name) {
   Module *M = BB->getParent()->getParent();
-  Function *Fn =  Intrinsic::getDeclaration(M, ID, { LHS->getType() });
+  Function *Fn = Intrinsic::getDeclaration(M, ID, { LHS->getType() });
   return createCallHelper(Fn, { LHS, RHS }, this, Name);
 }

lib/Transforms/InstCombine/InstCombineCalls.cpp

if (VWidth < 8) {
     Mask = Builder.CreateShuffleVector(Mask, Mask,
                                        makeArrayRef(Indices, VWidth),
                                        "extract");
   }
 
   return Builder.CreateSelect(Mask, Op0, Op1);
 }
 
-static Value *simplifyMinnumMaxnum(const IntrinsicInst &II) {
+static Value *simplifyMinnumMaxnum(const IntrinsicInst &II,
+                                   InstCombiner::BuilderTy &Builder) {
   Value *Arg0 = II.getArgOperand(0);
   Value *Arg1 = II.getArgOperand(1);
 
   // fmin(x, x) -> x
   if (Arg0 == Arg1)
-    return Arg0;
+    return Builder.CreateCanonicalize(Arg0);
 
   const auto *C1 = dyn_cast<ConstantFP>(Arg1);
 
   // fmin(x, nan) -> x
   if (C1 && C1->isNaN())
-    return Arg0;
+    return Builder.CreateCanonicalize(Arg0);
 
   // This is the value because if undef were NaN, we would return the other
   // value and cannot return a NaN unless both operands are.
   //
   // fmin(undef, x) -> x
   if (isa<UndefValue>(Arg0))
-    return Arg1;
+    return Builder.CreateCanonicalize(Arg1);
 
   // fmin(x, undef) -> x
   if (isa<UndefValue>(Arg1))
-    return Arg0;
+    return Builder.CreateCanonicalize(Arg0);
 
   Value *X = nullptr;
   Value *Y = nullptr;
   if (II.getIntrinsicID() == Intrinsic::minnum) {
     // fmin(x, fmin(x, y)) -> fmin(x, y)
     // fmin(y, fmin(x, y)) -> fmin(x, y)
     if (match(Arg1, m_FMin(m_Value(X), m_Value(Y)))) {
-      if (Arg0 == X || Arg0 == Y)
+      if (Arg0 == X || Arg0 == Y) {
         return Arg1;
       }
+    }
 
     // fmin(fmin(x, y), x) -> fmin(x, y)
     // fmin(fmin(x, y), y) -> fmin(x, y)
     if (match(Arg0, m_FMin(m_Value(X), m_Value(Y)))) {
       if (Arg1 == X || Arg1 == Y)
         return Arg0;
     }
 
     // TODO: fmin(nnan x, inf) -> x
     // TODO: fmin(nnan ninf x, flt_max) -> x
     if (C1 && C1->isInfinity()) {
       // fmin(x, -inf) -> -inf
       if (C1->isNegative())
-        return Arg1;
+        return Builder.CreateCanonicalize(Arg1);
     }
   } else {
     assert(II.getIntrinsicID() == Intrinsic::maxnum);
     // fmax(x, fmax(x, y)) -> fmax(x, y)
     // fmax(y, fmax(x, y)) -> fmax(x, y)
     if (match(Arg1, m_FMax(m_Value(X), m_Value(Y)))) {
       if (Arg0 == X || Arg0 == Y)
         return Arg1;
     }
 
     // fmax(fmax(x, y), x) -> fmax(x, y)
     // fmax(fmax(x, y), y) -> fmax(x, y)
     if (match(Arg0, m_FMax(m_Value(X), m_Value(Y)))) {
       if (Arg1 == X || Arg1 == Y)
         return Arg0;
     }
 
     // TODO: fmax(nnan x, -inf) -> x
     // TODO: fmax(nnan ninf x, -flt_max) -> x
     if (C1 && C1->isInfinity()) {
       // fmax(x, inf) -> inf
       if (!C1->isNegative())
-        return Arg1;
+        return Builder.CreateCanonicalize(Arg1);
     }
   }
   return nullptr;
 }
 
 static bool maskIsAllOneOrUndef(Value *Mask) {
   auto *ConstMask = dyn_cast<Constant>(Mask);
   if (!ConstMask)
case Intrinsic::maxnum: {
     Value *Arg0 = II->getArgOperand(0);
     Value *Arg1 = II->getArgOperand(1);
     // Canonicalize constants to the RHS.
     if (isa<ConstantFP>(Arg0) && !isa<ConstantFP>(Arg1)) {
       II->setArgOperand(0, Arg1);
       II->setArgOperand(1, Arg0);
       return II;
     }
-    if (Value *V = simplifyMinnumMaxnum(*II))
+    if (Value *V = simplifyMinnumMaxnum(*II, Builder))
       return replaceInstUsesWith(*II, V);
     break;
   }
   case Intrinsic::fmuladd: {
     // Canonicalize fast fmuladd to the separate fmul + fadd.
     if (II->hasUnsafeAlgebra()) {
       BuilderTy::FastMathFlagGuard Guard(Builder);
       Builder.setFastMathFlags(II->getFastMathFlags());

test/Transforms/InstCombine/AMDGPU/amdgcn-intrinsics.ll

 ; CHECK-LABEL: @fmed3_constant_src2_1_f32(
 ; CHECK: ret float 5.000000e-01
 define float @fmed3_constant_src2_1_f32(float %x, float %y) {
   %med3 = call float @llvm.amdgcn.fmed3.f32(float 4.0, float -1.0, float 0.5)
   ret float %med3
 }
 
 ; CHECK-LABEL: @fmed3_x_qnan0_qnan1_f32(
-; CHECK: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fmed3_x_qnan0_qnan1_f32(float %x) {
   %med3 = call float @llvm.amdgcn.fmed3.f32(float %x, float 0x7FF8001000000000, float 0x7FF8002000000000)
   ret float %med3
 }
 
 ; CHECK-LABEL: @fmed3_qnan0_x_qnan1_f32(
-; CHECK: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fmed3_qnan0_x_qnan1_f32(float %x) {
   %med3 = call float @llvm.amdgcn.fmed3.f32(float 0x7FF8001000000000, float %x, float 0x7FF8002000000000)
   ret float %med3
 }
 
 ; CHECK-LABEL: @fmed3_qnan0_qnan1_x_f32(
-; CHECK: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fmed3_qnan0_qnan1_x_f32(float %x) {
   %med3 = call float @llvm.amdgcn.fmed3.f32(float 0x7FF8001000000000, float 0x7FF8002000000000, float %x)
   ret float %med3
 }
 
 ; --------------------------------------------------------------------
 ; llvm.amdgcn.icmp
 ; --------------------------------------------------------------------

test/Transforms/InstCombine/maxnum.ll

 ; CHECK-LABEL: @canonicalize_constant_maxnum_f32
 ; CHECK: call float @llvm.maxnum.f32(float %x, float 1.000000e+00)
 define float @canonicalize_constant_maxnum_f32(float %x) #0 {
   %y = call float @llvm.maxnum.f32(float 1.0, float %x) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @noop_maxnum_f32
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @noop_maxnum_f32(float %x) #0 {
   %y = call float @llvm.maxnum.f32(float %x, float %x) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @maxnum_f32_nan_val
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @maxnum_f32_nan_val(float %x) #0 {
   %y = call float @llvm.maxnum.f32(float 0x7FF8000000000000, float %x) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @maxnum_f32_val_nan
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @maxnum_f32_val_nan(float %x) #0 {
   %y = call float @llvm.maxnum.f32(float %x, float 0x7FF8000000000000) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @fold_maxnum_f32_undef_undef
-; CHECK-NEXT: ret float undef
-define float @fold_maxnum_f32_undef_undef(float %x) nounwind {
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float undef)
+; CHECK-NEXT: ret float %1
+define float @fold_maxnum_f32_undef_undef() nounwind {
   %val = call float @llvm.maxnum.f32(float undef, float undef) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_maxnum_f32_val_undef
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fold_maxnum_f32_val_undef(float %x) nounwind {
   %val = call float @llvm.maxnum.f32(float %x, float undef) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_maxnum_f32_undef_val
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fold_maxnum_f32_undef_val(float %x) nounwind {
   %val = call float @llvm.maxnum.f32(float undef, float %x) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @maxnum_x_maxnum_x_y
 ; CHECK-NEXT: call float @llvm.maxnum.f32(float %x, float %y)
 ; CHECK-NEXT: ret float

 define float @maxnum4(float %x, float %y, float %z, float %w) #0 {
   %a = call float @llvm.maxnum.f32(float %x, float %y) #0
   %b = call float @llvm.maxnum.f32(float %z, float %w) #0
   %c = call float @llvm.maxnum.f32(float %a, float %b) #0
   ret float %c
 }
 
 ; CHECK-LABEL: @fold_maxnum_f32_inf_val
-; CHECK-NEXT: ret float 0x7FF0000000000000
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float 0x7FF0000000000000)
+; CHECK-NEXT: ret float %1
 define float @fold_maxnum_f32_inf_val(float %x) nounwind {
   %val = call float @llvm.maxnum.f32(float 0x7FF0000000000000, float %x) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_maxnum_f32_neginf_val
 ; CHECK-NEXT: call float @llvm.maxnum.f32(float %x, float 0xFFF0000000000000)
 ; CHECK-NEXT: ret float
 define float @fold_maxnum_f32_neginf_val(float %x) nounwind {
   %val = call float @llvm.maxnum.f32(float 0xFFF0000000000000, float %x) #0
   ret float %val
 }
 
 attributes #0 = { nounwind readnone }

test/Transforms/InstCombine/minnum.ll

 ; CHECK-LABEL: @canonicalize_constant_minnum_f32
 ; CHECK: call float @llvm.minnum.f32(float %x, float 1.000000e+00)
 define float @canonicalize_constant_minnum_f32(float %x) #0 {
   %y = call float @llvm.minnum.f32(float 1.0, float %x) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @noop_minnum_f32
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @noop_minnum_f32(float %x) #0 {
   %y = call float @llvm.minnum.f32(float %x, float %x) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @minnum_f32_nan_val
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @minnum_f32_nan_val(float %x) #0 {
   %y = call float @llvm.minnum.f32(float 0x7FF8000000000000, float %x) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @minnum_f32_val_nan
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @minnum_f32_val_nan(float %x) #0 {
   %y = call float @llvm.minnum.f32(float %x, float 0x7FF8000000000000) #0
   ret float %y
 }
 
 ; CHECK-LABEL: @fold_minnum_f32_undef_undef
-; CHECK-NEXT: ret float undef
-define float @fold_minnum_f32_undef_undef(float %x) nounwind {
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float undef)
+; CHECK-NEXT: ret float %1
+define float @fold_minnum_f32_undef_undef() nounwind {
   %val = call float @llvm.minnum.f32(float undef, float undef) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_minnum_f32_val_undef
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fold_minnum_f32_val_undef(float %x) nounwind {
   %val = call float @llvm.minnum.f32(float %x, float undef) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_minnum_f32_undef_val
-; CHECK-NEXT: ret float %x
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float %x)
+; CHECK-NEXT: ret float %1
 define float @fold_minnum_f32_undef_val(float %x) nounwind {
   %val = call float @llvm.minnum.f32(float undef, float %x) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @minnum_x_minnum_x_y
 ; CHECK-NEXT: call float @llvm.minnum.f32(float %x, float %y)
 ; CHECK-NEXT: ret float

 ; CHECK-NEXT: call float @llvm.minnum.f32(float %x, float 0x7FF0000000000000)
 ; CHECK-NEXT: ret float
 define float @fold_minnum_f32_inf_val(float %x) nounwind {
   %val = call float @llvm.minnum.f32(float 0x7FF0000000000000, float %x) #0
   ret float %val
 }
 
 ; CHECK-LABEL: @fold_minnum_f32_minf_val
-; CHECK-NEXT: ret float 0xFFF0000000000000
+; CHECK-NEXT: %1 = call float @llvm.canonicalize.f32(float 0xFFF0000000000000)
+; CHECK-NEXT: ret float %1
 define float @fold_minnum_f32_minf_val(float %x) nounwind {
   %val = call float @llvm.minnum.f32(float 0xFFF0000000000000, float %x) #0
   ret float %val
 }
 
 attributes #0 = { nounwind readnone }