[InstCombine] Fix mismatched attribute lists for combined calls

[guiand]

Summary

Currently, a transformation like pow(2.0, x) -> exp2(x) copies the pow
attribute list verbatim and applies it to exp2. This works out fine
when the attribute list is empty, but when it isn't clang will error due
due to the mismatch.

This was discovered after implementing the noundef param attribute.

[vitalybuka]

llvm/include/llvm/IR/Attributes.h

691 ↗

(On Diff #274279)

Is this

AttributeList LdexpAttrs = Attrs.shuffle<-1, 0, 1, 1>(B.getContext());

about the same as

AttributeList LdexpAttrs;
LdexpAttrs.addAttributes(Ctx, -1, Attrs.getAttributes(-1))
LdexpAttrs.addAttributes(Ctx, 0, Attrs.getAttributes(0))
LdexpAttrs.addAttributes(Ctx, 1, Attrs.getAttributes(1))
LdexpAttrs.addAttributes(Ctx, 2, Attrs.getAttributes(1))

I think later is easier to read than format.
maybe as a for loop

[jdoerfert]

Currently, a transformation like pow(2.0, x) -> exp2(x) copies the pow attribute list verbatim and applies it to exp2

For the record, this is clearly wrong even before the noundef patch. Thanks for fixing this.

I let you hash out the details of the shuffle thing ;)

(Ignore the libomp test failures)

[lebedev.ri]

llvm/include/llvm/IR/Attributes.h

691 ↗

(On Diff #274279)

I'm also worried about readability here.

[eugenis]

llvm/include/llvm/IR/Attributes.h

691 ↗

(On Diff #274279)

Agreed. I like the flexibility of Vitaly's example in case you need to go beyond a simple shuffle.

[guiand]

Addressed comments

[eugenis]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1459

When transforming pow(exp(x), y) to exp(x *y), why is it the right behavior to copy the attributes from the "y" parameter, and drop the ones on "x" and on "exp(x)"? Why not union or intersection?

It feels like the safe default behavior should be to drop all attributes, and adds the ones that we know apply here, like "noundef".

[guiand]

Addressed comments.

[guiand]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1459

You're definitely right, in the case of combining the pow parameters it doesn't really make sense to assign attributes from one or the other... maybe intersection makes sense though. I do still think it makes sense to copy attributes from x when we're eliminating a constant, like in pow(10.0, x) -> exp10(x). So I've split them up into two different attribute lists.

[guiand]

Fix oversight in previous change; pow(2.0**x) -> exp2 adds proper attributes now.

[vitalybuka]

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1810

does this assign attributes of x to the 1.0 constant?
why?

[guiand]

I was discussing with @eugenis about this and he suggested that it's reasonable not to copy any call-inst attributes when simplifying LibCalls. Specifically because you run into these messy questions of how reasonable it is to blindly apply attributes as a caller. Since LibCalls have their own inherent attribute lists which get applied upon creating a Function instance for them, it seems like we shouldn't impose any further attributes.

[eugenis]

This code generates a libcall out of thin air. My intuition says the safest thing to do is to drop all call site attributes, because they generally specify something about how an attribute must be passed to the callee, and not a property of the value being passed, so there is no reason for the attribute lists on pow and on exp to have anything in common at all.

This way we would lose the noundef attribute on the exp call arguments. We might extend TargetLibraryInfo in the future to specify attributes on the declarations.

WDYT?

[guiand]

This update removes attribute list copying per the discussion above.

[efriedma]

LGTM

[eugenis]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1788

This comment seems wrong. (intrinsics have attribute lists, libcalls don't)

[guiand]

Reworded comments to more closely match the reasoning

[eugenis]

LGTM

[guiand]

@eugenis and I discussed the possibility that removing attributes like this could cause problems if, for some reason, the ABI of a libcall changes as a result. The test I added actually demonstrates this: the inreg attribute is removed from the calls, which could change the behavior.

But this is only a concern if there's some reason to specify ABI-changing attributes to libcalls in the callsite like this, and not have LLVM put them in the declaration. Which I'm not sure there is. What are peoples' thoughts?

[efriedma]

If a libcall requires some ABI-changing attribute, presumably we'd make emitBinaryFloatFnCall take care of it, or something like that. I don't think float libcalls currently require ABI attributes on any target, but if they did, there isn't any reason to expect the attributes to be related.

[guiand]

Good to hear. Should this be good to go in that case?

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

static Value *getIntToFPVal(Value *I2F, IRBuilderBase &B) {
   return nullptr;
 }
 
 /// Use exp{,2}(x * y) for pow(exp{,2}(x), y);
 /// ldexp(1.0, x) for pow(2.0, itofp(x)); exp2(n * x) for pow(2.0 ** n, x);
 /// exp10(x) for pow(10.0, x); exp2(log2(n) * x) for pow(n, x).
 Value *LibCallSimplifier::replacePowWithExp(CallInst *Pow, IRBuilderBase &B) {
   Value *Base = Pow->getArgOperand(0), *Expo = Pow->getArgOperand(1);
-  AttributeList Attrs = Pow->getCalledFunction()->getAttributes();
+  AttributeList Attrs; // Attributes are only meaningful on the original call
   Module *Mod = Pow->getModule();
   Type *Ty = Pow->getType();
   bool Ignored;
 
   // Evaluate special cases related to a nested function as the base.
 
   // pow(exp(x), y) -> exp(x * y)
   // pow(exp2(x), y) -> exp2(x * y)
   // If exp{,2}() is used only once, it is better to fold two transcendental
   // math functions into one.  If used again, exp{,2}() would still have to be
   // called with the original argument, then keep both original transcendental
   // functions.  However, this transformation is only safe with fully relaxed
   // math semantics, since, besides rounding differences, it changes overflow
   // and underflow behavior quite dramatically.  For example:
   //   pow(exp(1000), 0.001) = pow(inf, 0.001) = inf
   // Whereas:
return emitUnaryFloatFnCall(V, TLI, LibFunc_sqrt, LibFunc_sqrtf,
                                 LibFunc_sqrtl, B, Attrs);
 
   return nullptr;
 }
 
 /// Use square root in place of pow(x, +/-0.5).
 Value *LibCallSimplifier::replacePowWithSqrt(CallInst *Pow, IRBuilderBase &B) {
   Value *Sqrt, *Base = Pow->getArgOperand(0), *Expo = Pow->getArgOperand(1);
-  AttributeList Attrs = Pow->getCalledFunction()->getAttributes();
+  AttributeList Attrs; // Attributes are only meaningful on the original call
   Module *Mod = Pow->getModule();
   Type *Ty = Pow->getType();
 
   const APFloat *ExpoF;
   if (!match(Expo, m_APFloat(ExpoF)) ||
       (!ExpoF->isExactlyValue(0.5) && !ExpoF->isExactlyValue(-0.5)))
     return nullptr;
 
if (Value *ExpoI = getIntToFPVal(Expo, B))
       return createPowWithIntegerExponent(Base, ExpoI, M, B);
   }
 
   return Shrunk;
 }
 
 Value *LibCallSimplifier::optimizeExp2(CallInst *CI, IRBuilderBase &B) {
   Function *Callee = CI->getCalledFunction();
+  AttributeList Attrs; // Attributes are only meaningful on the original call
   StringRef Name = Callee->getName();
   Value *Ret = nullptr;
   if (UnsafeFPShrink && Name == TLI->getName(LibFunc_exp2) &&
       hasFloatVersion(Name))
     Ret = optimizeUnaryDoubleFP(CI, B, true);
 
   Type *Ty = CI->getType();
   Value *Op = CI->getArgOperand(0);
 
   // Turn exp2(sitofp(x)) -> ldexp(1.0, sext(x))  if sizeof(x) <= 32
   // Turn exp2(uitofp(x)) -> ldexp(1.0, zext(x))  if sizeof(x) < 32
   if ((isa<SIToFPInst>(Op) || isa<UIToFPInst>(Op)) &&
       hasFloatFn(TLI, Ty, LibFunc_ldexp, LibFunc_ldexpf, LibFunc_ldexpl)) {
     if (Value *Exp = getIntToFPVal(Op, B))
       return emitBinaryFloatFnCall(ConstantFP::get(Ty, 1.0), Exp, TLI,
                                    LibFunc_ldexp, LibFunc_ldexpf, LibFunc_ldexpl,
-                                   B, CI->getCalledFunction()->getAttributes());
+                                   B, Attrs);
   }
 
   return Ret;
 }
 
 Value *LibCallSimplifier::optimizeFMinFMax(CallInst *CI, IRBuilderBase &B) {
   // If we can shrink the call to a float function rather than a double
   // function, do that first.
   Function *Callee = CI->getCalledFunction();
   StringRef Name = Callee->getName();
   if ((Name == "fmin" || Name == "fmax") && hasFloatVersion(Name))
     if (Value *Ret = optimizeBinaryDoubleFP(CI, B))
       return Ret;
Value *LibCallSimplifier::optimizeFMinFMax(CallInst *CI, IRBuilderBase &B) {
   Intrinsic::ID IID = Callee->getName().startswith("fmin") ? Intrinsic::minnum
                                                            : Intrinsic::maxnum;
   Function *F = Intrinsic::getDeclaration(CI->getModule(), IID, CI->getType());
   return B.CreateCall(F, { CI->getArgOperand(0), CI->getArgOperand(1) });
 }
 
 Value *LibCallSimplifier::optimizeLog(CallInst *Log, IRBuilderBase &B) {
   Function *LogFn = Log->getCalledFunction();
-  AttributeList Attrs = LogFn->getAttributes();
+  AttributeList Attrs; // Attributes are only meaningful on the original call
   StringRef LogNm = LogFn->getName();
   Intrinsic::ID LogID = LogFn->getIntrinsicID();
   Module *Mod = Log->getModule();
   Type *Ty = Log->getType();
   Value *Ret = nullptr;
 
   if (UnsafeFPShrink && hasFloatVersion(LogNm))
     Ret = optimizeUnaryDoubleFP(Log, B, true);

llvm/test/Transforms/InstCombine/pow_fp_int.ll

 ;
   %subfp = uitofp i31 %x to double
   %pow = tail call afn double @llvm.pow.f64(double 7.000000e+00, double %subfp)
   ret double %pow
 }
 
 define double @pow_sitofp_double_const_base_2_fast(i32 %x) {
 ; CHECK-LABEL: @pow_sitofp_double_const_base_2_fast(
-; CHECK-NEXT:    [[LDEXPF:%.*]] = call afn float @ldexpf(float 1.000000e+00, i32 [[X:%.*]]) #1
+; CHECK-NEXT:    [[LDEXPF:%.*]] = call afn float @ldexpf(float 1.000000e+00, i32 [[X:%.*]])
 ; CHECK-NEXT:    [[RES:%.*]] = fpext float [[LDEXPF]] to double
 ; CHECK-NEXT:    ret double [[RES]]
 ;
   %subfp = sitofp i32 %x to float
   %pow = tail call afn float @llvm.pow.f32(float 2.000000e+00, float %subfp)
   %res = fpext float %pow to double
   ret double %res
 }
;
   %pow = tail call afn float @llvm.pow.f32(float 16.000000e+00, float %subfp)
   %res = fpext float %pow to double
   ret double %res
 }
 
 define double @pow_uitofp_const_base_2_fast(i31 %x) {
 ; CHECK-LABEL: @pow_uitofp_const_base_2_fast(
 ; CHECK-NEXT:    [[TMP1:%.*]] = zext i31 [[X:%.*]] to i32
-; CHECK-NEXT:    [[LDEXPF:%.*]] = call afn float @ldexpf(float 1.000000e+00, i32 [[TMP1]]) #1
+; CHECK-NEXT:    [[LDEXPF:%.*]] = call afn float @ldexpf(float 1.000000e+00, i32 [[TMP1]])
 ; CHECK-NEXT:    [[RES:%.*]] = fpext float [[LDEXPF]] to double
 ; CHECK-NEXT:    ret double [[RES]]
 ;
   %subfp = uitofp i31 %x to float
   %pow = tail call afn float @llvm.pow.f32(float 2.000000e+00, float %subfp)
   %res = fpext float %pow to double
   ret double %res
 }
;
   %subfp = uitofp i32 %x to float
   %pow = tail call float @llvm.pow.f32(float 7.000000e+00, float %subfp)
   %res = fpext float %pow to double
   ret double %res
 }
 
 define double @pow_sitofp_const_base_2_no_fast(i32 %x) {
 ; CHECK-LABEL: @pow_sitofp_const_base_2_no_fast(
-; CHECK-NEXT:    [[LDEXPF:%.*]] = call float @ldexpf(float 1.000000e+00, i32 [[X:%.*]]) #1
+; CHECK-NEXT:    [[LDEXPF:%.*]] = call float @ldexpf(float 1.000000e+00, i32 [[X:%.*]])
 ; CHECK-NEXT:    [[RES:%.*]] = fpext float [[LDEXPF]] to double
 ; CHECK-NEXT:    ret double [[RES]]
 ;
   %subfp = sitofp i32 %x to float
   %pow = tail call float @llvm.pow.f32(float 2.000000e+00, float %subfp)
   %res = fpext float %pow to double
   ret double %res
 }

llvm/test/Transforms/InstCombine/simplify-libcalls.ll

 ; CHECK-LABEL: @fake_toascii(
 ; CHECK-NEXT:    [[Y:%.*]] = call i32 @toascii(i8 %x)
 ; CHECK-NEXT:    ret i32 [[Y]]
 ;
   %y = call i32 @toascii(i8 %x)
   ret i32 %y
 }
 
+declare double @pow(double, double)
+declare double @exp2(double)
+
+; check to make sure only the correct libcall attributes are used
+define double @fake_exp2(double %x) {
+; CHECK-LABEL: @fake_exp2(
+; CHECK-NEXT:    [[Y:%.*]] = call double @exp2(double %x)
+; CHECK-NEXT:    ret double [[Y]]
+
+  %y = call inreg double @pow(double inreg 2.0, double inreg %x)
+  ret double %y
+}
+define double @fake_ldexp(i32 %x) {
+; CHECK-LABEL: @fake_ldexp(
+; CHECK-NEXT:    [[Z:%.*]] = call double @ldexp(double 1.0{{.*}}, i32 %x)
+; CHECK-NEXT:    ret double [[Z]]
+
+  %y = sitofp i32 %x to double
+  %z = call inreg double @exp2(double %y)
+  ret double %z
+}
+
 
 attributes #0 = { nobuiltin }
 attributes #1 = { builtin }