[InstCombiner] Make LibCallSimplifier add extension attribute to ldexp arg.

[jonpa]

Summary

When an exp2() or or pow() is replaced by a call to ldexp(), the second integer argument should have the signext or zeroext attribute set (depending
on the conversion instruction opcode preceding the original call).

This was discovered by the SystemZ multistage bot: http://lab.llvm.org:8011/#/builders/8, where wrong code resulted when this extension was not performed.

[uweigand]

This doesn't look quite right to me. ldexp has a signed "int" argument, so you always must use the "sext" sign extension attribute on that argument.

[Note that transforming a pow (2.0, uitofp (...)) to an ldexp is only even valid if the unsigned integer type is smaller than 32 bits. But this seems to be handled correctly by getIntToFPVal.]

[jonpa]

I see... Patch updated.

[lebedev.ri]

This doesn't look right to me.
This should be done elsewhere more generically.
See the rest of SimplifyLibCalls.cpp - we should simply annotate ldexp's args with appropriate signext attribue.

[jonpa]

In D90760#2373757, @lebedev.ri wrote:

This doesn't look right to me.
This should be done elsewhere more generically.
See the rest of SimplifyLibCalls.cpp - we should simply annotate ldexp's args with appropriate signext attribue.

I find cases of "CI->addParamAttr(ArgNo, ...)", but I thought also the declaration of ldexp should as well have the signext attribute on the argument, not just on the call instruction operand. Or is that not needed? (clang emits it also on the prototype for a normal function, it seems). Passing the attributes like I did makes emitBinaryFloatFnCallHelper() add them to both 'Callee' and 'CI'...

[jonpa]

ping

@lebedev.ri Thanks for review so far. Since this is keeping a SystemZ buildbot failing, I would appreciate some further advice here... thanks

[lebedev.ri]

In D90760#2382819, @jonpa wrote:

ping

@lebedev.ri Thanks for review so far. Since this is keeping a SystemZ buildbot failing, I would appreciate some further advice here... thanks

Given a function declaration, (e.g. that @ldexp), will/should all calls agree how the argument should be extended?

[jonpa]

In D90760#2382851, @lebedev.ri wrote:

In D90760#2382819, @jonpa wrote:

ping

@lebedev.ri Thanks for review so far. Since this is keeping a SystemZ buildbot failing, I would appreciate some further advice here... thanks

Given a function declaration, (e.g. that @ldexp), will/should all calls agree how the argument should be extended?

I believe all calls to @ldexp should have the signext attribute. It is then up to the target to perform the sign extension if needed per the calling convention. The attribute is needed to inform the target what kind of extension should be performed (sign/zero).

[lebedev.ri]

In D90760#2382857, @jonpa wrote:

In D90760#2382851, @lebedev.ri wrote:

In D90760#2382819, @jonpa wrote:

ping

@lebedev.ri Thanks for review so far. Since this is keeping a SystemZ buildbot failing, I would appreciate some further advice here... thanks

Given a function declaration, (e.g. that @ldexp), will/should all calls agree how the argument should be extended?

I believe all calls to @ldexp should have the signext attribute.

Awesome.
So, can you try if the miscompile goes away if you manually annotate the @ldexp declaration in IR with said signext?

It is then up to the target to perform the sign extension if needed per the calling convention. The attribute is needed to inform the target what kind of extension should be performed (sign/zero).

[uweigand]

I'm not sure whether the call instruction must match the declaration w.r.t. extension attributes. In the IR generated by clang, the two always match:

; Function Attrs: noinline nounwind optnone
define dso_local signext i32 @test(i32 signext %x) #0 {
entry:
  %x.addr = alloca i32, align 4
  store i32 %x, i32* %x.addr, align 4
  %0 = load i32, i32* %x.addr, align 4
  ret i32 %0
}

; Function Attrs: noinline nounwind optnone
define dso_local signext i32 @main() #0 {
entry:
  %retval = alloca i32, align 4
  store i32 0, i32* %retval, align 4
  %0 = load i32, i32* @a, align 4
  %call = call signext i32 @test(i32 signext %0)
  ret i32 %call
}

I'm not sure what is supposed to happen if the two do not match, this doesn't seem obvious from reading the IR specs ...

[jonpa]

So, can you try if the miscompile goes away if you manually annotate the @ldexp declaration in IR with said signext?

It seems that it is enough to have either the signext on the call operand or in the declaration, for the extension to be emitted by the SystemZ backend.

[lebedev.ri]

In D90760#2383015, @jonpa wrote:

So, can you try if the miscompile goes away if you manually annotate the @ldexp declaration in IR with said signext?

It seems that it is enough to have either the signext on the call operand or in the declaration, for the extension to be emitted by the SystemZ backend.

Great.
So back to my original idea - can we simply annotate the LibFunc_ldexp*'s argument with signext attribute?
@xbolva00 might be able to help.

[jonpa]

In D90760#2383035, @lebedev.ri wrote:

In D90760#2383015, @jonpa wrote:

So, can you try if the miscompile goes away if you manually annotate the @ldexp declaration in IR with said signext?

It seems that it is enough to have either the signext on the call operand or in the declaration, for the extension to be emitted by the SystemZ backend.

Great.
So back to my original idea - can we simply annotate the LibFunc_ldexp*'s argument with signext attribute?
@xbolva00 might be able to help.

Are we sure that this is generally acceptable despite the fact that clang emits the signext attribute in both places?

If so, I suppose we should do something like what is done in insertSinCosCall(), i.e. calling M->getOrInsertFunction() and build the call instead of calling emitBinaryFloatFnCall()? That doesn't seem simpler to me though than the current patch...

[jonpa]

So back to my original idea - can we simply annotate the LibFunc_ldexp*'s argument with signext attribute?

Ah, now I think I finally understand what you mean... :-) I agree this seems much better... Patch updated.

[lebedev.ri]

Yep, that is indeed what i was suggesting all along :)
LGTM, thank you.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

 //- Infer Attributes ---------------------------------------------------------//
 
 STATISTIC(NumReadNone, "Number of functions inferred as readnone");
 STATISTIC(NumReadOnly, "Number of functions inferred as readonly");
 STATISTIC(NumArgMemOnly, "Number of functions inferred as argmemonly");
 STATISTIC(NumNoUnwind, "Number of functions inferred as nounwind");
 STATISTIC(NumNoCapture, "Number of arguments inferred as nocapture");
 STATISTIC(NumWriteOnlyArg, "Number of arguments inferred as writeonly");
+STATISTIC(NumSExtArg, "Number of arguments inferred as signext");
 STATISTIC(NumReadOnlyArg, "Number of arguments inferred as readonly");
 STATISTIC(NumNoAlias, "Number of function returns inferred as noalias");
 STATISTIC(NumNoUndef, "Number of function returns inferred as noundef returns");
 STATISTIC(NumNonNull, "Number of function returns inferred as nonnull returns");
 STATISTIC(NumReturnedArg, "Number of arguments inferred as returned");
 
 static bool setDoesNotAccessMemory(Function &F) {
   if (F.doesNotAccessMemory())

 static bool setOnlyWritesMemory(Function &F, unsigned ArgNo) {
   if (F.hasParamAttribute(ArgNo, Attribute::WriteOnly))
     return false;
   F.addParamAttr(ArgNo, Attribute::WriteOnly);
   ++NumWriteOnlyArg;
   return true;
 }
 
+static bool setSignExtendedArg(Function &F, unsigned ArgNo) {
+ if (F.hasParamAttribute(ArgNo, Attribute::SExt))
+    return false;
+  F.addParamAttr(ArgNo, Attribute::SExt);
+  ++NumSExtArg;
+  return true;
+}
 
 static bool setRetNoUndef(Function &F) {
   if (!F.getReturnType()->isVoidTy() &&
       !F.hasAttribute(AttributeList::ReturnIndex, Attribute::NoUndef)) {
     F.addAttribute(AttributeList::ReturnIndex, Attribute::NoUndef);
     ++NumNoUndef;
     return true;
   }
case LibFunc_memset_pattern16:
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   // int __nvvm_reflect(const char *)
   case LibFunc_nvvm_reflect:
     Changed |= setRetAndArgsNoUndef(F);
     Changed |= setDoesNotAccessMemory(F);
     Changed |= setDoesNotThrow(F);
     return Changed;
-
+  case LibFunc_ldexp:
+  case LibFunc_ldexpf:
+  case LibFunc_ldexpl:
+    Changed |= setSignExtendedArg(F, 1);
+    return Changed;
   default:
     // FIXME: It'd be really nice to cover all the library functions we're
     // aware of here.
     return false;
   }
 }
 
 bool llvm::hasFloatFn(const TargetLibraryInfo *TLI, Type *Ty,
Value *llvm::emitUnaryFloatFnCall(Value *Op, const TargetLibraryInfo *TLI,
   StringRef Name = getFloatFnName(TLI, Op->getType(),
                                   DoubleFn, FloatFn, LongDoubleFn);
 
   return emitUnaryFloatFnCallHelper(Op, Name, B, Attrs);
 }
 
 static Value *emitBinaryFloatFnCallHelper(Value *Op1, Value *Op2,
                                           StringRef Name, IRBuilderBase &B,
-                                          const AttributeList &Attrs) {
+                                          const AttributeList &Attrs,
+                                          const TargetLibraryInfo *TLI = nullptr) {
   assert((Name != "") && "Must specify Name to emitBinaryFloatFnCall");
 
   Module *M = B.GetInsertBlock()->getModule();
   FunctionCallee Callee = M->getOrInsertFunction(Name, Op1->getType(),
                                                  Op1->getType(), Op2->getType());
+  if (TLI != nullptr)
+    inferLibFuncAttributes(M, Name, *TLI);
   CallInst *CI = B.CreateCall(Callee, { Op1, Op2 }, Name);
 
   // The incoming attribute set may have come from a speculatable intrinsic, but
   // is being replaced with a library call which is not allowed to be
   // speculatable.
   CI->setAttributes(Attrs.removeAttribute(B.getContext(),
                                           AttributeList::FunctionIndex,
                                           Attribute::Speculatable));
Value *llvm::emitBinaryFloatFnCall(Value *Op1, Value *Op2,
                                    const TargetLibraryInfo *TLI,
                                    LibFunc DoubleFn, LibFunc FloatFn,
                                    LibFunc LongDoubleFn, IRBuilderBase &B,
                                    const AttributeList &Attrs) {
   // Get the name of the function according to TLI.
   StringRef Name = getFloatFnName(TLI, Op1->getType(),
                                   DoubleFn, FloatFn, LongDoubleFn);
 
-  return emitBinaryFloatFnCallHelper(Op1, Op2, Name, B, Attrs);
+  return emitBinaryFloatFnCallHelper(Op1, Op2, Name, B, Attrs, TLI);
 }
 
 Value *llvm::emitPutChar(Value *Char, IRBuilderBase &B,
                          const TargetLibraryInfo *TLI) {
   if (!TLI->has(LibFunc_putchar))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();

llvm/test/Transforms/InferFunctionAttrs/annotate.ll

 declare i32 @isdigit(i32)
 
 ; CHECK: declare i64 @labs(i64) [[G0]]
 declare i64 @labs(i64)
 
 ; CHECK: declare noundef i32 @lchown(i8* nocapture noundef readonly, i32 noundef, i32 noundef) [[G1]]
 declare i32 @lchown(i8*, i32, i32)
 
-; CHECK: declare double @ldexp(double, i32) [[G0]]
+; CHECK: declare double @ldexp(double, i32 signext) [[G0]]
 declare double @ldexp(double, i32)
 
-; CHECK: declare float @ldexpf(float, i32) [[G0]]
+; CHECK: declare float @ldexpf(float, i32 signext) [[G0]]
 declare float @ldexpf(float, i32)
 
-; CHECK: declare x86_fp80 @ldexpl(x86_fp80, i32) [[G0]]
+; CHECK: declare x86_fp80 @ldexpl(x86_fp80, i32 signext) [[G0]]
 declare x86_fp80 @ldexpl(x86_fp80, i32)
 
 ; CHECK: declare i64 @llabs(i64) [[G0]]
 declare i64 @llabs(i64)
 
 ; CHECK: declare double @log(double) [[G0]]
 declare double @log(double)