InstCombine: Only propagate callsite attributes in sqrt->sqrtf

[arsenm]

This was propagating the callee's attributes instead of just the
callsite. It's illegal to set denormal_fpenv on a callsite. This
was also losing callsite attributes which may have been more useful;
there's no point in setting the callee's attributes on the callsite.

[arsenm]

• InstCombine: Only propagate callsite attributes in sqrt->sqrtf #180160 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Matt Arsenault (arsenm)

Changes

This was propagating the callee's attributes instead of just the
callsite. It's illegal to set denormal_fpenv on a callsite. This
was also losing callsite attributes which may have been more useful;
there's no point in setting the callee's attributes on the callsite.

---

Full diff: https://github.com/llvm/llvm-project/pull/180160.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp (+5-4)
• (modified) llvm/test/Transforms/InstCombine/double-float-shrink-1.ll (+49-23)
• (modified) llvm/test/Transforms/InstCombine/sqrt.ll (+3-3)

diff --git a/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp b/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
index 1e8a184fc51b8..14d82b5d8813c 100644
--- a/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
@@ -2015,10 +2015,11 @@ static Value *optimizeDoubleFP(CallInst *CI, IRBuilderBase &B,
     R = isBinary ? B.CreateIntrinsic(IID, B.getFloatTy(), V)
                  : B.CreateIntrinsic(IID, B.getFloatTy(), V[0]);
   } else {
-    AttributeList CalleeAttrs = CalleeFn->getAttributes();
-    R = isBinary ? emitBinaryFloatFnCall(V[0], V[1], TLI, CalleeName, B,
-                                         CalleeAttrs)
-                 : emitUnaryFloatFnCall(V[0], TLI, CalleeName, B, CalleeAttrs);
+    AttributeList CallsiteAttrs = CI->getAttributes();
+    R = isBinary
+            ? emitBinaryFloatFnCall(V[0], V[1], TLI, CalleeName, B,
+                                    CallsiteAttrs)
+            : emitUnaryFloatFnCall(V[0], TLI, CalleeName, B, CallsiteAttrs);
   }
   return B.CreateFPExt(R, B.getDoubleTy());
 }
diff --git a/llvm/test/Transforms/InstCombine/double-float-shrink-1.ll b/llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
index c13a01bb9d489..3b1d1ad016a4f 100644
--- a/llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
+++ b/llvm/test/Transforms/InstCombine/double-float-shrink-1.ll
@@ -546,33 +546,59 @@ define float @fake_fmin(float %a, float %b) {
   ret float %f
 }
 
-declare fp128 @fmin(fp128, fp128)
+; The combine from sqrt->sqrtf should only propagate the existing
+; callsite attributes. It should not propgate the callee's function
+; attributes to the callsite; it's invalid to set denormal_fpenv on the callsiten
+define void @sqrt_to_sqrtf_only_preserve_callsite_attrs() #0 {
+; LINUX-LABEL: define void @sqrt_to_sqrtf_only_preserve_callsite_attrs(
+; LINUX-SAME: ) #[[ATTR0:[0-9]+]] {
+; LINUX-NEXT:  [[ENTRY:.*:]]
+; LINUX-NEXT:    [[SQRTF:%.*]] = call float @sqrtf(float noundef -1.000000e+00) #[[CALLSITE_ATTR:[0-9]+]]
+; LINUX-NEXT:    ret void
+entry:
+  %call = call double @sqrt(double noundef -1.000000e+00) #2, !tbaa !0
+  ret void
+}
+
+declare fp128 @fmin(fp128, fp128) #0
 
-declare double @fmax(double, double)
+declare double @fmax(double, double) #0
 
-declare double @tanh(double)
-declare double @tan(double)
+declare double @tanh(double) #0
+declare double @tan(double) #0
 
 ; sqrt is a special case: the shrinking optimization
 ; is valid even without unsafe-fp-math.
-declare double @sqrt(double)
+declare double @sqrt(double) #0
 declare double @llvm.sqrt.f64(double)
 
-declare double @sin(double)
-declare double @pow(double, double)
-declare double @log2(double)
-declare double @log1p(double)
-declare double @log10(double)
-declare double @log(double)
-declare double @logb(double)
-declare double @exp10(double)
-declare double @expm1(double)
-declare double @exp(double)
-declare double @cbrt(double)
-declare double @atanh(double)
-declare double @atan(double)
-declare double @acos(double)
-declare double @acosh(double)
-declare double @asin(double)
-declare double @asinh(double)
-
+declare double @sin(double) #0
+declare double @pow(double, double) #0
+declare double @log2(double) #0
+declare double @log1p(double) #0
+declare double @log10(double) #0
+declare double @log(double) #0
+declare double @logb(double) #0
+declare double @exp10(double) #0
+declare double @expm1(double) #0
+declare double @exp(double) #0
+declare double @cbrt(double) #0
+declare double @atanh(double) #0
+declare double @atan(double) #0
+declare double @acos(double) #0
+declare double @acosh(double) #0
+declare double @asin(double) #0
+declare double @asinh(double) #0
+
+; LINUX: #[[CALLSITE_ATTR]] = { "custom-callsite-attr" }
+
+attributes #0 = { denormal_fpenv(float: dynamic) }
+attributes #1 = { mustprogress nocallback nofree nounwind willreturn denormal_fpenv(float: preservesign) memory(errnomem: write) }
+attributes #2 = { "custom-callsite-attr" }
+
+!llvm.errno.tbaa = !{!0}
+
+!0 = !{!1, !1, i64 0}
+!1 = !{!"int", !2, i64 0}
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C/C++ TBAA"}
diff --git a/llvm/test/Transforms/InstCombine/sqrt.ll b/llvm/test/Transforms/InstCombine/sqrt.ll
index 2fda5bc37d023..0a1aa21c04696 100644
--- a/llvm/test/Transforms/InstCombine/sqrt.ll
+++ b/llvm/test/Transforms/InstCombine/sqrt.ll
@@ -16,7 +16,7 @@ define float @test1(float %x) nounwind readnone ssp {
 
 define float @test2(float %x) nounwind readnone ssp {
 ; CHECK-LABEL: @test2(
-; CHECK-NEXT:    [[SQRTF:%.*]] = call float @sqrtf(float [[X:%.*]]) #[[ATTR4]]
+; CHECK-NEXT:    [[SQRTF:%.*]] = call float @sqrtf(float [[X:%.*]]) #[[ATTR5:[0-9]+]]
 ; CHECK-NEXT:    ret float [[SQRTF]]
 ;
   %conv = fpext float %x to double
@@ -32,7 +32,7 @@ define float @test2(float %x) nounwind readnone ssp {
 define float @test3(ptr %v) nounwind uwtable ssp {
 ; CHECK-LABEL: @test3(
 ; CHECK-NEXT:    [[CALL34:%.*]] = call double @sqrt(double 0x7FF8000000000000) #[[ATTR4]]
-; CHECK-NEXT:    [[CALL36:%.*]] = call i32 @foo(double [[CALL34]]) #[[ATTR5:[0-9]+]]
+; CHECK-NEXT:    [[CALL36:%.*]] = call i32 @foo(double [[CALL34]]) #[[ATTR5]]
 ; CHECK-NEXT:    [[CONV38:%.*]] = fptrunc double [[CALL34]] to float
 ; CHECK-NEXT:    ret float [[CONV38]]
 ;
@@ -51,7 +51,7 @@ define float @test3(ptr %v) nounwind uwtable ssp {
 
 define void @0(float %f) {
 ; CHECK-LABEL: @0(
-; CHECK-NEXT:    [[SQRTF:%.*]] = call float @sqrtf(float [[F:%.*]]) #[[ATTR2:[0-9]+]]
+; CHECK-NEXT:    [[SQRTF:%.*]] = call float @sqrtf(float [[F:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   %d = fpext float %f to double

[nikic]

LGTM. I checked the other getAttributes() calls in the file and it looks like this is the only problematic one.

[nikic]

I'd prefer this to be a separate test instead of adding denormal_fpenv to the existing one.

[arsenm]

I figured it's better to add it to the declarations of all the other cases to make sure those are all OK