Fix sign extension for MIPS64 in makeLibCall function

[spetrovic]

Summary

Fixing sign extension in makeLibCall for MIPS64. In MIPS64 architecture all 32 bit arguments (int, unsigned int, float 32 (soft float)) must be sign extended. This fixes test "MultiSource/Applications/oggenc/"

[hfinkel]

This is target-independent code, what does, "This should not affect other architectures." mean? Maybe you need a new target callback? Why does this affect only FRINT and not also FCEIL, FTRUNC, FNEARBYINT, FROUND, FFLOOR?

test/CodeGen/Mips/mips64rintfsf.ll

21 ↗

(On Diff #20406)

Please remove any unnecessary attributes.

[spetrovic]

I think you are right about target callback, did you have something like this in mind ? I currently found the problem in this function. The problem exist in other functions, but I haven't seen it manifest yet. I will do some extra tests on those functions next. I have seen GCC and LLVM (for other cases) do sign extension on float arguments.

[spetrovic]

Fixed white spaces.

[petarj]

include/llvm/Target/TargetLowering.h

1074 ↗

(On Diff #21578)

Can you please add a comment before the function?

lib/Target/Mips/MipsISelLowering.cpp

3034 ↗

(On Diff #21578)

Nit: remove empty line.

3035 ↗

(On Diff #21578)

I believe you should be checking for Subtarget.abiUsesSoftFloat() too.
Upper 32bits are undefined for hard float ABI.

3036 ↗

(On Diff #21578)

Remove empty line.

lib/Target/Mips/MipsISelLowering.h

477 ↗

(On Diff #21578)

Add an empty line above this.

[spetrovic]

Comments addressed.

[spetrovic]

Test updated because of changes on LLVM.

[hfinkel]

Regarding the other functions, is there a reason why the obvious modification to the regression test for this one won't work?

Generically, I'd like to see as much testing coverage as possible; when you make the change, I think that it should be done for all of the functions in a consistent manner. Please test as much as you can, but if there are some you can't on your target, that's okay.

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

494 ↗

(On Diff #21591)

Why did you name the callback shouldSignExtendFloat32InAbiCall when the use is not specific to f32? I recommend having the callback take the type, and taking Float32 out of the name.

[petarj]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

494 ↗

(On Diff #21591)

The need for this change comes from MIPS64 decision to keep 32-bit values in registers by sign-extending them to 64 bits.
Maybe the function name should reflect that?

[hfinkel]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

494 ↗

(On Diff #21591)

No, this is target-independent code.

[spetrovic]

makeLibCall function is patched, now every function argument and return value are sign extended if needed. In MIPS64 architecture all 32 bit arguments (int, unsigned int, float 32) must be sign extended. In makeLibCall function 32 bit arguments are converted to int32, so there is check if is any MIPS64 architecture, soft float ABI, and int 32 type then do sign extension. This patch is only for MIPS64 and doesn't affect other architectures.

[hfinkel]

include/llvm/Target/TargetLowering.h

1075 ↗

(On Diff #22252)

This is certainly better, my only issue now is with the function name. The term 'ABI call' seems ambiguous (and appears in the Mips backend, but nowhere else in the same way), and this specifically applies to calls generated with makeLibCall. How about naming it?

shouldSignExtendTypeInLibCall

which seems to fit better with general LLVM terminology.

[spetrovic]

Function name changed to shouldSignExtendTypeInLibCall.

[hfinkel]

LGTM, thanks!

llvm/trunk/include/llvm/Target/TargetLowering.h

public:
   /// @}
 
   /// Returns true if the given (atomic) store should be expanded by the
   /// IR-level AtomicExpand pass into an "atomic xchg" which ignores its input.
   virtual bool shouldExpandAtomicStoreInIR(StoreInst *SI) const {
     return false;
   }
 
+  /// Returns true if arguments should be sign-extended in lib calls.
+  virtual bool shouldSignExtendTypeInLibCall(EVT Type, bool IsSigned) const {
+    return IsSigned;
+ }
+
   /// Returns true if the given (atomic) load should be expanded by the
   /// IR-level AtomicExpand pass into a load-linked instruction
   /// (through emitLoadLinked()).
   virtual bool shouldExpandAtomicLoadInIR(LoadInst *LI) const { return false; }
 
   /// Returns how the IR-level AtomicExpand pass should expand the given
   /// AtomicRMW, if at all. Default is to never expand.
   virtual AtomicRMWExpansionKind

llvm/trunk/lib/CodeGen/SelectionDAG/TargetLowering.cpp

TargetLowering::makeLibCall(SelectionDAG &DAG,
                             bool isReturnValueUsed) const {
   TargetLowering::ArgListTy Args;
   Args.reserve(NumOps);
 
   TargetLowering::ArgListEntry Entry;
   for (unsigned i = 0; i != NumOps; ++i) {
     Entry.Node = Ops[i];
     Entry.Ty = Entry.Node.getValueType().getTypeForEVT(*DAG.getContext());
-    Entry.isSExt = isSigned;
-    Entry.isZExt = !isSigned;
+    Entry.isSExt = shouldSignExtendTypeInLibCall(Ops[i].getValueType(), isSigned);
+    Entry.isZExt = !shouldSignExtendTypeInLibCall(Ops[i].getValueType(), isSigned);
     Args.push_back(Entry);
   }
   SDValue Callee = DAG.getExternalSymbol(getLibcallName(LC), getPointerTy());
 
   Type *RetTy = RetVT.getTypeForEVT(*DAG.getContext());
   TargetLowering::CallLoweringInfo CLI(DAG);
+  bool signExtend = shouldSignExtendTypeInLibCall(RetVT, isSigned);
   CLI.setDebugLoc(dl).setChain(DAG.getEntryNode())
     .setCallee(getLibcallCallingConv(LC), RetTy, Callee, std::move(Args), 0)
     .setNoReturn(doesNotReturn).setDiscardResult(!isReturnValueUsed)
-    .setSExtResult(isSigned).setZExtResult(!isSigned);
+    .setSExtResult(signExtend).setZExtResult(!signExtend);
   return LowerCallTo(CLI);
 }
 
 
 /// SoftenSetCCOperands - Soften the operands of a comparison.  This code is
 /// shared among BR_CC, SELECT_CC, and SETCC handlers.
 void TargetLowering::softenSetCCOperands(SelectionDAG &DAG, EVT VT,
                                          SDValue &NewLHS, SDValue &NewRHS,

llvm/trunk/lib/Target/Mips/MipsISelLowering.h

bool CanLowerReturn(CallingConv::ID CallConv, MachineFunction &MF,
                         LLVMContext &Context) const override;
 
     SDValue LowerReturn(SDValue Chain,
                         CallingConv::ID CallConv, bool isVarArg,
                         const SmallVectorImpl<ISD::OutputArg> &Outs,
                         const SmallVectorImpl<SDValue> &OutVals,
                         SDLoc dl, SelectionDAG &DAG) const override;
 
+    bool shouldSignExtendTypeInLibCall(EVT Type, bool IsSigned) const override;
+
     // Inline asm support
     ConstraintType
       getConstraintType(const std::string &Constraint) const override;
 
     /// Examine constraint string and operand type and determine a weight value.
     /// The operand object must already have been set up with the operand type.
     ConstraintWeight getSingleConstraintMatchWeight(
       AsmOperandInfo &info, const char *constraint) const override;

llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp

MipsTargetLowering::CanLowerReturn(CallingConv::ID CallConv,
                                    MachineFunction &MF, bool IsVarArg,
                                    const SmallVectorImpl<ISD::OutputArg> &Outs,
                                    LLVMContext &Context) const {
   SmallVector<CCValAssign, 16> RVLocs;
   MipsCCState CCInfo(CallConv, IsVarArg, MF, RVLocs, Context);
   return CCInfo.CheckReturn(Outs, RetCC_Mips);
 }
 
+bool
+MipsTargetLowering::shouldSignExtendTypeInLibCall(EVT Type, bool IsSigned) const {
+  if (Subtarget.hasMips3() && Subtarget.abiUsesSoftFloat()) {
+    if (Type == MVT::i32)
+      return true;
+  }
+  return IsSigned;
+}
+
 SDValue
 MipsTargetLowering::LowerReturn(SDValue Chain,
                                 CallingConv::ID CallConv, bool IsVarArg,
                                 const SmallVectorImpl<ISD::OutputArg> &Outs,
                                 const SmallVectorImpl<SDValue> &OutVals,
                                 SDLoc DL, SelectionDAG &DAG) const {
   // CCValAssign - represent the assignment of
   // the return value to a location

llvm/trunk/test/CodeGen/Mips/mips64-f128.ll

 define fp128 @load_LD_LD() {
 entry:
   %0 = load fp128, fp128* @gld1, align 16
   ret fp128 %0
 }
 
 ; ALL-LABEL: load_LD_float:
 ; ALL: ld   $[[R0:[0-9]+]], %got_disp(gf1)
-; ALL: lwu  $4, 0($[[R0]])
+; ALL: lw   $4, 0($[[R0]])
 ; ALL: ld   $25, %call16(__extendsftf2)
 ; ALL: jalr $25
 
 define fp128 @load_LD_float() {
 entry:
   %0 = load float, float* @gf1, align 4
   %conv = fpext float %0 to fp128
   ret fp128 %conv

llvm/trunk/test/CodeGen/Mips/mips64signextendsesf.ll

+; RUN: llc -march=mips64 -mcpu=mips64r2 -soft-float -O2 < %s | FileCheck %s
+
+define void @foosf() #0 {
+entry:
+  %in = alloca float, align 4
+  %out = alloca float, align 4
+  store volatile float 0xBFD59E1380000000, float* %in, align 4
+  %in.0.in.0. = load volatile float, float* %in, align 4
+  %rintf = tail call float @rintf(float %in.0.in.0.) #1
+  store volatile float %rintf, float* %out, align 4
+  ret void
+
+; CHECK-LABEL:      foosf
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @rintf(float)
+
+define float @foosf1(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %call = tail call float @roundf(float %0) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf1
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @roundf(float) #1
+
+define float @foosf2(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %call = tail call float @truncf(float %0) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf2
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @truncf(float) #1
+
+define float @foosf3(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %call = tail call float @floorf(float %0) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf3
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @floorf(float) #1
+
+define float @foosf4(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %call = tail call float @nearbyintf(float %0) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf4
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @nearbyintf(float) #1
+
+define float @foosf5(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %mul = fmul float %0, undef
+  ret float %mul
+
+; CHECK-LABEL:      foosf5
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+define float @foosf6(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %sub = fsub float %0, undef
+  ret float %sub
+
+; CHECK-LABEL:      foosf6
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+define float @foosf7(float* nocapture readonly %a) #0 {
+entry:
+  %0 = load float, float* %a, align 4
+  %add = fadd float %0, undef
+  ret float %add
+
+; CHECK-LABEL:      foosf7
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+define float @foosf8(float* nocapture readonly %a) #0 {
+entry:
+  %b = alloca float, align 4
+  %b.0.b.0. = load volatile float, float* %b, align 4
+  %0 = load float, float* %a, align 4
+  %div = fdiv float %b.0.b.0., %0
+  ret float %div
+
+; CHECK-LABEL:      foosf8
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+define float @foosf9() #0 {
+entry:
+  %b = alloca float, align 4
+  %b.0.b.0. = load volatile float, float* %b, align 4
+  %conv = fpext float %b.0.b.0. to double
+  %b.0.b.0.3 = load volatile float, float* %b, align 4
+  %conv1 = fpext float %b.0.b.0.3 to double
+  %call = tail call double @pow(double %conv, double %conv1) #1
+  %conv2 = fptrunc double %call to float
+  ret float %conv2
+
+; CHECK-LABEL:      foosf9
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare double @pow(double, double) #0
+
+define float @foosf10() #0 {
+entry:
+  %a = alloca float, align 4
+  %a.0.a.0. = load volatile float, float* %a, align 4
+  %conv = fpext float %a.0.a.0. to double
+  %call = tail call double @sin(double %conv) #1
+  %conv1 = fptrunc double %call to float
+  ret float %conv1
+
+; CHECK-LABEL:      foosf10
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare double @sin(double) #0
+
+define float @foosf11() #0 {
+entry:
+  %b = alloca float, align 4
+  %b.0.b.0. = load volatile float, float* %b, align 4
+  %call = tail call float @ceilf(float %b.0.b.0.) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf11
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @ceilf(float) #1
+
+define float @foosf12() #0 {
+entry:
+  %b = alloca float, align 4
+  %a = alloca float, align 4
+  %b.0.b.0. = load volatile float, float* %b, align 4
+  %a.0.a.0. = load volatile float, float* %a, align 4
+  %call = tail call float @fmaxf(float %b.0.b.0., float %a.0.a.0.) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf12
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @fmaxf(float, float) #1
+
+define float @foosf13() #0 {
+entry:
+  %b = alloca float, align 4
+  %a = alloca float, align 4
+  %b.0.b.0. = load volatile float, float* %b, align 4
+  %a.0.a.0. = load volatile float, float* %a, align 4
+  %call = tail call float @fminf(float %b.0.b.0., float %a.0.a.0.) #2
+  ret float %call
+
+; CHECK-LABEL:      foosf13
+; CHECK-NOT:        dsll
+; CHECK-NOT:        dsrl
+; CHECK-NOT:        lwu
+}
+
+declare float @fminf(float, float) #1
+
+
+attributes #0 = { nounwind "use-soft-float"="true" }
+attributes #1 = { nounwind readnone "use-soft-float"="true" }
 No newline at end of file