[BuildLibCalls][RISCV] Sign extend return value of bcmp on riscv64.

[craig.topper]

Summary

riscv64 wants callees to sign extend signed and unsigned int returns.

The caller can use this to avoid a sign extend if the result is
used by a comparison since riscv64 only has 64-bit compares.

InstCombine/SimplifyLibCalls aggressively turn memcmps that are only
used by an icmp eq 0 into bcmp, but we lose the sext attribute that
would have been present on the memcmp. This causes an unneeded sext.w
in the generated assembly.

This looks even sillier if bcmp is implemented alias to memcmp. In
that case, not only did we not get any savings by using bcmp, we added
an instruction.

This probably applies to other functions, this just happens to be
the one I noticed so far.

See also the discussion here https://discourse.llvm.org/t/can-we-preserve-signext-return-attribute-when-converting-memcmp-to-bcmp/67126

[jrtc27]

Isn't MIPS the same? RISC-V just inherited this from MIPS by being a derivative of it (both ISA-wise and in the GNU toolchain implementation).

[jrtc27]

Hm, maybe not, GCC and Clang both seem to want to normalise the upper bits for dealing with an unsigned 32-bit return value being promoted to 64-bit, whether signed or unsigned... and the psABI document is really unclear on what on earth the convention is for return values in most respects. It's clear on parameters at least...

[craig.topper]

In D139901#3990628, @jrtc27 wrote:

Hm, maybe not, GCC and Clang both seem to want to normalise the upper bits for dealing with an unsigned 32-bit return value being promoted to 64-bit, whether signed or unsigned... and the psABI document is really unclear on what on earth the convention is for return values in most respects. It's clear on parameters at least...

It looks like TargetInfo.cpp in clang applies the attributes to integer arguments regardless of triple. See MipsABIInfo::extendType.

// MIPS64 ABI requires unsigned 32 bit integers to be sign extended.           
if (Ty->isUnsignedIntegerOrEnumerationType() && TySize == 32)                  
  return ABIArgInfo::getSignExtend(Ty);                                        
                                                                               
return ABIArgInfo::getExtend(Ty);

For returns only applies to if the triple arch is mips64 or mips64el. See the bottom of MipsABIInfo::classifyReturnType.

if ((RetTy->isUnsignedIntegerOrEnumerationType() ||                            
    RetTy->isSignedIntegerOrEnumerationType()) && Size == 32 && !IsO32)        
  return ABIArgInfo::getSignExtend(RetTy);

Note the !IsO32 that was not present in MipsABIInfo::extendType

[craig.topper]

Ping. Does this look ok for RISC-V?

[efriedma]

LGTM

llvm/include/llvm/Analysis/TargetLibraryInfo.h

 /// depends on the triple. So users typically interact with the \c
 /// TargetLibraryInfo wrapper below.
 class TargetLibraryInfoImpl {
   friend class TargetLibraryInfo;
 
   unsigned char AvailableArray[(NumLibFuncs+3)/4];
   DenseMap<unsigned, std::string> CustomNames;
   static StringLiteral const StandardNames[NumLibFuncs];
-  bool ShouldExtI32Param, ShouldExtI32Return, ShouldSignExtI32Param;
+  bool ShouldExtI32Param, ShouldExtI32Return, ShouldSignExtI32Param, ShouldSignExtI32Return;
   unsigned SizeOfInt;
 
   enum AvailabilityState {
     StandardName = 3, // (memset to all ones)
     CustomName = 1,
     Unavailable = 0  // (memset to all zeros)
   };
   void setState(LibFunc F, AvailabilityState State) {
public:
   }
 
   /// Set to true iff i32 parameters to library functions should have signext
   /// attribute if they correspond to C-level int or unsigned int.
   void setShouldSignExtI32Param(bool Val) {
     ShouldSignExtI32Param = Val;
   }
 
+  /// Set to true iff i32 results from library functions should have signext
+  /// attribute if they correspond to C-level int or unsigned int.
+  void setShouldSignExtI32Return(bool Val) {
+    ShouldSignExtI32Return = Val;
+  }
+
   /// Returns the size of the wchar_t type in bytes or 0 if the size is unknown.
   /// This queries the 'wchar_size' metadata.
   unsigned getWCharSize(const Module &M) const;
 
   /// Returns the size of the size_t type in bits.
   unsigned getSizeTSize(const Module &M) const;
 
   /// Get size of a C-level int or unsigned int, in bits.
public:
   }
 
   /// Returns extension attribute kind to be used for i32 return values
   /// corresponding to C-level int or unsigned int.  May be zeroext, signext,
   /// or none.
   Attribute::AttrKind getExtAttrForI32Return(bool Signed = true) const {
     if (Impl->ShouldExtI32Return)
       return Signed ? Attribute::SExt : Attribute::ZExt;
+    if (Impl->ShouldSignExtI32Return)
+      return Attribute::SExt;
     return Attribute::None;
   }
 
   /// \copydoc TargetLibraryInfoImpl::getWCharSize()
   unsigned getWCharSize(const Module &M) const {
     return Impl->getWCharSize(M);
   }
 

llvm/lib/Analysis/TargetLibraryInfo.cpp

static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
   TLI.setUnavailable(LibFunc_fputc_unlocked);
   TLI.setUnavailable(LibFunc_fgetc_unlocked);
   TLI.setUnavailable(LibFunc_fread_unlocked);
   TLI.setUnavailable(LibFunc_fwrite_unlocked);
   TLI.setUnavailable(LibFunc_fputs_unlocked);
   TLI.setUnavailable(LibFunc_fgets_unlocked);
 
   bool ShouldExtI32Param = false, ShouldExtI32Return = false,
-       ShouldSignExtI32Param = false;
+       ShouldSignExtI32Param = false, ShouldSignExtI32Return = false;
   // PowerPC64, Sparc64, SystemZ need signext/zeroext on i32 parameters and
   // returns corresponding to C-level ints and unsigned ints.
   if (T.isPPC64() || T.getArch() == Triple::sparcv9 ||
       T.getArch() == Triple::systemz) {
     ShouldExtI32Param = true;
     ShouldExtI32Return = true;
   }
   // Mips and riscv64, on the other hand, needs signext on i32 parameters
   // corresponding to both signed and unsigned ints.
   if (T.isMIPS() || T.isRISCV64()) {
     ShouldSignExtI32Param = true;
   }
+  // riscv64 needs signext on i32 returns corresponding to both signed and
+  // unsigned ints.
+  if (T.isRISCV64()) {
+    ShouldSignExtI32Return = true;
+  }
   TLI.setShouldExtI32Param(ShouldExtI32Param);
   TLI.setShouldExtI32Return(ShouldExtI32Return);
   TLI.setShouldSignExtI32Param(ShouldSignExtI32Param);
+  TLI.setShouldSignExtI32Return(ShouldSignExtI32Return);
 
   // Let's assume by default that the size of int is 32 bits, unless the target
   // is a 16-bit architecture because then it most likely is 16 bits. If that
   // isn't true for a target those defaults should be overridden below.
   TLI.setIntSize(T.isArch16Bit() ? 16 : 32);
 
   // There is really no runtime library on AMDGPU, apart from
   // __kmpc_alloc/free_shared.
TargetLibraryInfoImpl::TargetLibraryInfoImpl(const Triple &T) {
 
   initialize(*this, T, StandardNames);
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(const TargetLibraryInfoImpl &TLI)
     : CustomNames(TLI.CustomNames), ShouldExtI32Param(TLI.ShouldExtI32Param),
       ShouldExtI32Return(TLI.ShouldExtI32Return),
       ShouldSignExtI32Param(TLI.ShouldSignExtI32Param),
+      ShouldSignExtI32Return(TLI.ShouldSignExtI32Return),
       SizeOfInt(TLI.SizeOfInt) {
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
   VectorDescs = TLI.VectorDescs;
   ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl::TargetLibraryInfoImpl(TargetLibraryInfoImpl &&TLI)
     : CustomNames(std::move(TLI.CustomNames)),
       ShouldExtI32Param(TLI.ShouldExtI32Param),
       ShouldExtI32Return(TLI.ShouldExtI32Return),
       ShouldSignExtI32Param(TLI.ShouldSignExtI32Param),
+      ShouldSignExtI32Return(TLI.ShouldSignExtI32Return),
       SizeOfInt(TLI.SizeOfInt) {
   std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
             AvailableArray);
   VectorDescs = TLI.VectorDescs;
   ScalarDescs = TLI.ScalarDescs;
 }
 
 TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(const TargetLibraryInfoImpl &TLI) {
   CustomNames = TLI.CustomNames;
   ShouldExtI32Param = TLI.ShouldExtI32Param;
   ShouldExtI32Return = TLI.ShouldExtI32Return;
   ShouldSignExtI32Param = TLI.ShouldSignExtI32Param;
+  ShouldSignExtI32Return = TLI.ShouldSignExtI32Return;
   SizeOfInt = TLI.SizeOfInt;
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
   return *this;
 }
 
 TargetLibraryInfoImpl &TargetLibraryInfoImpl::operator=(TargetLibraryInfoImpl &&TLI) {
   CustomNames = std::move(TLI.CustomNames);
   ShouldExtI32Param = TLI.ShouldExtI32Param;
   ShouldExtI32Return = TLI.ShouldExtI32Return;
   ShouldSignExtI32Param = TLI.ShouldSignExtI32Param;
+  ShouldSignExtI32Return = TLI.ShouldSignExtI32Return;
   SizeOfInt = TLI.SizeOfInt;
   std::move(std::begin(TLI.AvailableArray), std::end(TLI.AvailableArray),
             AvailableArray);
   return *this;
 }
 
 static StringRef sanitizeFunctionName(StringRef funcName) {
   // Filter out empty names and names containing null bytes, those can't be in

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

 
 static void setArgExtAttr(Function &F, unsigned ArgNo,
                           const TargetLibraryInfo &TLI, bool Signed = true) {
   Attribute::AttrKind ExtAttr = TLI.getExtAttrForI32Param(Signed);
   if (ExtAttr != Attribute::None && !F.hasParamAttribute(ArgNo, ExtAttr))
     F.addParamAttr(ArgNo, ExtAttr);
 }
 
+static void setRetExtAttr(Function &F,
+                          const TargetLibraryInfo &TLI, bool Signed = true) {
+  Attribute::AttrKind ExtAttr = TLI.getExtAttrForI32Return(Signed);
+  if (ExtAttr != Attribute::None && !F.hasRetAttribute(ExtAttr))
+    F.addRetAttr(ExtAttr);
+}
+
 // Modeled after X86TargetLowering::markLibCallAttributes.
 static void markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
     return;
 
   const CallingConv::ID CC = F->getCallingConv();
   if (CC != CallingConv::C && CC != CallingConv::X86_StdCall)
     return;
FunctionCallee llvm::getOrInsertLibFunc(Module *M, const TargetLibraryInfo &TLI,
   case LibFunc_memccpy:
     setArgExtAttr(*F, 2, TLI);
     break;
 
     // These are functions that are known to not need any argument extension
     // on any target: A size_t argument (which may be an i32 on some targets)
     // should not trigger the assert below.
   case LibFunc_bcmp:
+    setRetExtAttr(*F, TLI);
+    break;
   case LibFunc_calloc:
   case LibFunc_fwrite:
   case LibFunc_malloc:
   case LibFunc_memcmp:
   case LibFunc_memcpy_chk:
   case LibFunc_mempcpy:
   case LibFunc_memset_pattern16:
   case LibFunc_snprintf:

llvm/test/Transforms/InstCombine/RISCV/memcmp.ll

+; RUN: opt %s -passes=instcombine -mtriple=riscv64-unknown-linux-gnu -S | FileCheck %s
+
+declare signext i32 @memcmp(ptr, ptr, i64)
+
+; Make sure we use signext attribute for the bcmp result.
+define signext i32 @test_bcmp(ptr %mem1, ptr %mem2, i64 %size) {
+; CHECK-LABEL: define {{[^@]+}}@test_bcmp(
+; CHECK-NEXT:    [[BCMP:%.*]] = call i32 @bcmp(ptr [[MEM1:%.*]], ptr [[MEM2:%.*]], i64 [[SIZE:%.*]])
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[BCMP]], 0
+; CHECK-NEXT:    [[ZEXT:%.*]] = zext i1 [[CMP]] to i32
+; CHECK-NEXT:    ret i32 [[ZEXT]]
+;
+  %call = call signext i32 @memcmp(ptr %mem1, ptr %mem2, i64 %size)
+  %cmp = icmp eq i32 %call, 0
+  %zext = zext i1 %cmp to i32
+  ret i32 %zext
+}
+
+; CHECK: declare signext i32 @bcmp(ptr nocapture, ptr nocapture, i64)