[RISCV] Avoid generating AssertZext for LP64 ABI when lowering floating Libcall

[shiva0217]

Summary

The patch fixed the issue that RV64 didn't clear the upper bits when return complex floating value with lp64 ABI.

float _Complex
complex_add(float _Complex a, float _Complex b)
{
return a + b;
}

RealResult = zero_extend(RealA + RealB)
ImageResult = ImageA + ImageB
Return (RealResult | (ImageResult << 32))

The patch introduces shouldExtendTypeInLibCall target hook to suppress
the AssertZext generation when lowering floating LibCall for LP64 ABI.

Thanks to Eli's comments from the Bugzilla
https://bugs.llvm.org/show_bug.cgi?id=42820

[lenary]

I thought the problem was in that in the complex add case, we needed to zero-extend the i32 into an i64, in order to zero the upper 32 bits. This patch seems to sign-extend the libcall instead. Is there a shouldZeroExtendTypeInLibCall callback?

[shiva0217]

In D65497#1607839, @lenary wrote:

I thought the problem was in that in the complex add case, we needed to zero-extend the i32 into an i64, in order to zero the upper 32 bits. This patch seems to sign-extend the libcall instead. Is there a shouldZeroExtendTypeInLibCall callback?

Hi @lenary,

It seems that shouldSignExtendTypeInLibCall control the value is signed or zero extented in TargetLowering::makeLibCall.

...
bool signExtend = shouldSignExtendTypeInLibCall(RetVT, isSigned);
...
CLI.setDebugLoc(dl)
    .setChain(DAG.getEntryNode())
    .setLibCallee(getLibcallCallingConv(LC), RetTy, Callee, std::move(Args))
    .setNoReturn(doesNotReturn)
    .setDiscardResult(!isReturnValueUsed)
    .setSExtResult(signExtend)
    .setZExtResult(!signExtend);

The IR before the or operation:

t42: i64 = AssertSext t40, ValueType:ch:i32
t45: i64 = and t42, Constant:i64<4294967295>
t23: i64 = or t21, t45

If shouldSignExtendTypeInLibCall return true in this case, AssertSext will be generated and the and operation for zero_extend will be preserved.
If shouldSignExtendTypeInLibCall return false, AssertZext will be generated and the and operation for zero_extend will be eliminated.

[efriedma]

I don't think there's any way to define shouldSignExtendTypeInLibCall that completely solves the issue without target-independent changes. Yes, it's true that floats should not be zero-extended, but they also shouldn't be sign-extended. So makeLibCall should do something equivalent to .setSExtResult(false).setZExtResult(false) for the result, and Entry.IsSExt = false; Entry.IsZExt = false; for the arguments. And this needs to apply only to float arguments/results; integer arguments and results need to be sign-extended from 32 bits to 64 bits. (Not sure we currently generate any libcalls with 32-bit arguments/results on RV64, but that could change in the future.)

You might need to pass more information into TargetLowering::makeLibCall, or add a new entry point.

[shiva0217]

Hi @eli.friedman,

Thanks for pointing me the right direction.
I added shouldExtendTypeInLibCall target hook to indicate the Libcall should do the extension or not.
The parameter IsCastFromFloat will be passed to TargetLowering::makeLibCall and shouldExtendTypeInLibCall. So shouldExtendTypeInLibCall can identify the Type is casting by floating and disable the extensions.
Without generating AssertZext for the Libcall return value, and operation for clearing upper bits will be preserved.

[kito-cheng]

test/CodeGen/RISCV/rv32i-rv64i-float-double.ll

39

Why this two extension is not gone? I thought this patch should also improve divsf3 too?

[shiva0217]

test/CodeGen/RISCV/rv32i-rv64i-float-double.ll

39

Hi Kito,
I think you're right, it should be able to remove. I'll update the patch to catch the case, Thanks.

[shiva0217]

Update the patch to catch divsf3 case.

[efriedma]

I think this is semantically the correct fix, but I have some concern about the way the APIs are written.

Also, I'm concerned about the way you're finding the calls to makeLibCall that need to be modified to use "float" mode. Fixing them one by one as you find examples in assembly, rather than auditing all the calls, is going to lead to a stream of difficult-to-find bugs.

include/llvm/CodeGen/TargetLowering.h

1834 ↗

(On Diff #212952)

Can the target actually do anything useful with the "IsExtended" boolean? I think it makes more sense to just write something like IsExtended && shouldExtendTypeInLibCall(Type, IsCastFromFloat) in the caller.

3033 ↗

(On Diff #212952)

makeLibCall now takes five booleans, four of which are optional. This is completely unreadable.

I can see a few ways to fix this. One, make the arguments non-optional, and use enum classes to name each one appropriately. Two, introduce a makeLibCallOptions struct, with some appropriate defaults and methods to override each one, sort of like CallLoweringInfo. Or three, introduce more entry points with appropriate names.

Also, I think you actually need to separately handle the return value and the argument; for example, float-to-int conversion has a non-extended argument, and a sign-extended result. I don't think we generate calls to __fixsfsi at the moment on rv64; we call __fixsfdi instead. But I'd rather fix the API to do the right thing, given we're changing it anyway.

test/CodeGen/RISCV/rv64-complex-float.ll

133 ↗

(On Diff #212952)

More unnecessary shifts.

[shiva0217]

include/llvm/CodeGen/TargetLowering.h

3033 ↗

(On Diff #212952)

Hi @efriedma,

I have created a parent patch D65795 to remove the optional arguments.
The patch introduces makeLibCallOptions struct which contains ArgListEntry and CallLoweringInfo classes. Each class has its appropriate constructor to initial default flags. We could remove optional arguments by passing makeLibCallOptions struct.
With this changing, argument flags could specify by ArgListEntry class and return value flag could specify by CallLoweringInfo class.
Is the patch could meet what you thought in your mind? Thanks.

[shiva0217]

include/llvm/CodeGen/TargetLowering.h

1834 ↗

(On Diff #212952)

Removing "IsExtended" from argument lists seems much cleaner, thanks. :)

test/CodeGen/RISCV/rv64-complex-float.ll

133 ↗

(On Diff #212952)

Thanks for catching this.

[shiva0217]

Rebase the patch base on D65795 and update test cases.
Thanks for Eli's comments and Alex's excellent floating test cases that we could easily observe the difference.

[efriedma]

The approach seems better; I'll comment in more detail on the other review.

I'm still concerned you're not taking the time to go through the code to find all the places which need to be marked as using float arguments.

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

133 ↗

(On Diff #213807)

unreachable?

test/CodeGen/RISCV/float-fcmp.ll

7 ↗

(On Diff #213807)

Please commit the new RUN line separately.

[shiva0217]

Hi @efriedma,

I remove the IsCastFromFloat flag and store the SDNode before soften in MakeLibCallOptions struct. So that the shouldExtendTypeInLibCall could get the original type directly and get rid of the complicate IsCastFromFloat setting method. I try to illustrate most of the single soft-float functions in rv64i-single-softfloat.ll. Please kindly remind me if I still missing something, thanks.

[luismarques]

include/llvm/CodeGen/TargetLowering.h

3560 ↗

(On Diff #214309)

This new field makes the parent struct a bit misnamed, and something about this approach of passing the node in the options smells a bit to me, but I couldn't come up with a better alternative. Also, for alignment purposes it's probably better to put the SDNode pointer before the flags.

lib/CodeGen/SelectionDAG/TargetLowering.cpp

137 ↗

(On Diff #214309)

No point in calling shouldSignExtendTypeInLibCall if shouldExt is false. Refactor the flow?

[shiva0217]

include/llvm/CodeGen/TargetLowering.h

3560 ↗

(On Diff #214309)

Agree, the parent struct becomes a bit misnamed with the new field, but I can't come up a better way, either. I'll move up the new field, thanks.

lib/CodeGen/SelectionDAG/TargetLowering.cpp

137 ↗

(On Diff #214309)

Thanks for catching this. It seems much cleaner.

[shiva0217]

Update patch to reflect the comments.

Hi @luismarques, thanks for the review.

[efriedma]

include/llvm/CodeGen/TargetLowering.h

3560 ↗

(On Diff #214309)

something about this approach of passing the node in the options smells a bit to me

I agree... the problem is essentially that it's tying MakeLibCallOptions to the specific usage in legalization. In general, the API should take an "original" return type, and a list of "original" argument types. That said, transforming the node into that form is a little awkward, so I think it's fine to leave it, with a comment that it could be generalized if necessary.

[shiva0217]

Add comment for the new field in MakeLibCallOptions struct.

Hi @efriedma, thanks for your guidance.

[luismarques]

The main concerns have been addressed.
LGTM, assuming the typo in the comment is fixed.

include/llvm/CodeGen/TargetLowering.h

3558 ↗

(On Diff #216309)

typo: "necessory" -> "necessary"

[miyuki]

Hi. Could you please look into https://bugs.llvm.org/show_bug.cgi?id=43183 (it seems to be related)

lib/Target/RISCV/RISCVISelLowering.h

public:
   unsigned
   getExceptionPointerRegister(const Constant *PersonalityFn) const override;
 
   /// If a physical register, this returns the register that receives the
   /// exception typeid on entry to a landing pad.
   unsigned
   getExceptionSelectorRegister(const Constant *PersonalityFn) const override;
 
+  bool shouldSignExtendTypeInLibCall(EVT Type, bool IsSigned) const override;
+
 private:
   void analyzeInputArgs(MachineFunction &MF, CCState &CCInfo,
                         const SmallVectorImpl<ISD::InputArg> &Ins,
                         bool IsRet) const;
   void analyzeOutputArgs(MachineFunction &MF, CCState &CCInfo,
                          const SmallVectorImpl<ISD::OutputArg> &Outs,
                          bool IsRet, CallLoweringInfo *CLI) const;
   // Lower incoming arguments, copy physregs into vregs

lib/Target/RISCV/RISCVISelLowering.cpp

unsigned RISCVTargetLowering::getExceptionPointerRegister(
     const Constant *PersonalityFn) const {
   return RISCV::X10;
 }
 
 unsigned RISCVTargetLowering::getExceptionSelectorRegister(
     const Constant *PersonalityFn) const {
   return RISCV::X11;
 }
+
+bool RISCVTargetLowering::shouldSignExtendTypeInLibCall(EVT Type,
+                                                        bool IsSigned) const {
+  RISCVABI::ABI ABI = Subtarget.getTargetABI();
+  if ((ABI == RISCVABI::ABI_LP64) && Type == MVT::i32)
+    return true;
+
+  return IsSigned;
+}

test/CodeGen/RISCV/calling-conv-lp64.ll

 
 define i64 @callee_float_in_regs(i64 %a, float %b) nounwind {
 ; RV64I-FPELIM-LABEL: callee_float_in_regs:
 ; RV64I-FPELIM:       # %bb.0:
 ; RV64I-FPELIM-NEXT:    addi sp, sp, -16
 ; RV64I-FPELIM-NEXT:    sd ra, 8(sp)
 ; RV64I-FPELIM-NEXT:    sd s0, 0(sp)
 ; RV64I-FPELIM-NEXT:    mv s0, a0
-; RV64I-FPELIM-NEXT:    slli a0, a1, 32
-; RV64I-FPELIM-NEXT:    srli a0, a0, 32
+; RV64I-FPELIM-NEXT:    sext.w a0, a1
 ; RV64I-FPELIM-NEXT:    call __fixsfdi
 ; RV64I-FPELIM-NEXT:    add a0, s0, a0
 ; RV64I-FPELIM-NEXT:    ld s0, 0(sp)
 ; RV64I-FPELIM-NEXT:    ld ra, 8(sp)
 ; RV64I-FPELIM-NEXT:    addi sp, sp, 16
 ; RV64I-FPELIM-NEXT:    ret
 ;
 ; RV64I-WITHFP-LABEL: callee_float_in_regs:
 ; RV64I-WITHFP:       # %bb.0:
 ; RV64I-WITHFP-NEXT:    addi sp, sp, -32
 ; RV64I-WITHFP-NEXT:    sd ra, 24(sp)
 ; RV64I-WITHFP-NEXT:    sd s0, 16(sp)
 ; RV64I-WITHFP-NEXT:    sd s1, 8(sp)
 ; RV64I-WITHFP-NEXT:    addi s0, sp, 32
 ; RV64I-WITHFP-NEXT:    mv s1, a0
-; RV64I-WITHFP-NEXT:    slli a0, a1, 32
-; RV64I-WITHFP-NEXT:    srli a0, a0, 32
+; RV64I-WITHFP-NEXT:    sext.w a0, a1
 ; RV64I-WITHFP-NEXT:    call __fixsfdi
 ; RV64I-WITHFP-NEXT:    add a0, s1, a0
 ; RV64I-WITHFP-NEXT:    ld s1, 8(sp)
 ; RV64I-WITHFP-NEXT:    ld s0, 16(sp)
 ; RV64I-WITHFP-NEXT:    ld ra, 24(sp)
 ; RV64I-WITHFP-NEXT:    addi sp, sp, 32
 ; RV64I-WITHFP-NEXT:    ret
   %b_fptosi = fptosi float %b to i64

test/CodeGen/RISCV/rv32i-rv64i-float-double.ll

 ; RV32IF-NEXT:    addi sp, sp, 16
 ; RV32IF-NEXT:    ret
 ;
 ; RV64IF-LABEL: float_test:
 ; RV64IF:       # %bb.0:
 ; RV64IF-NEXT:    addi sp, sp, -16
 ; RV64IF-NEXT:    sd ra, 8(sp)
 ; RV64IF-NEXT:    sd s0, 0(sp)
-; RV64IF-NEXT:    slli a0, a0, 32
-; RV64IF-NEXT:    srli a0, a0, 32
-; RV64IF-NEXT:    slli a1, a1, 32
-; RV64IF-NEXT:    srli s0, a1, 32
+; RV64IF-NEXT:    sext.w a0, a0
+; RV64IF-NEXT:    sext.w s0, a1
 ; RV64IF-NEXT:    mv a1, s0
 ; RV64IF-NEXT:    call __addsf3
 ; RV64IF-NEXT:    mv a1, s0
 ; RV64IF-NEXT:    call __divsf3
 ; RV64IF-NEXT:    ld s0, 0(sp)
 ; RV64IF-NEXT:    ld ra, 8(sp)
 ; RV64IF-NEXT:    addi sp, sp, 16
 ; RV64IF-NEXT:    ret
   %1 = fadd float %a, %b
   %2 = fdiv float %1, %b
   ret float %2
 }

test/CodeGen/RISCV/rv64-complex-floating-add.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s | FileCheck %s
+
+; The test case check that zero_extensions will be generated when lowering
+; single floating operations.
+; Please reference https://bugs.llvm.org/show_bug.cgi?id=42820 for more details.
+
+define i64 @complex_float_add(i64 %a.coerce, i64 %b.coerce) nounwind {
+; CHECK-LABEL: complex_float_add:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    addi sp, sp, -32
+; CHECK-NEXT:    sd ra, 24(sp)
+; CHECK-NEXT:    sd s0, 16(sp)
+; CHECK-NEXT:    sd s1, 8(sp)
+; CHECK-NEXT:    sd s2, 0(sp)
+; CHECK-NEXT:    mv s0, a1
+; CHECK-NEXT:    mv s1, a0
+; CHECK-NEXT:    sext.w a0, a0
+; CHECK-NEXT:    sext.w a1, a1
+; CHECK-NEXT:    call __addsf3
+; CHECK-NEXT:    mv s2, a0
+; CHECK-NEXT:    srai a0, s1, 32
+; CHECK-NEXT:    srai a1, s0, 32
+; CHECK-NEXT:    call __addsf3
+; CHECK-NEXT:    slli a0, a0, 32
+; CHECK-NEXT:    slli a1, s2, 32
+; CHECK-NEXT:    srli a1, a1, 32
+; CHECK-NEXT:    or a0, a0, a1
+; CHECK-NEXT:    ld s2, 0(sp)
+; CHECK-NEXT:    ld s1, 8(sp)
+; CHECK-NEXT:    ld s0, 16(sp)
+; CHECK-NEXT:    ld ra, 24(sp)
+; CHECK-NEXT:    addi sp, sp, 32
+; CHECK-NEXT:    ret
+entry:
+  %a.sroa.0.0.extract.trunc = trunc i64 %a.coerce to i32
+  %0 = bitcast i32 %a.sroa.0.0.extract.trunc to float
+  %a.sroa.2.0.extract.shift = lshr i64 %a.coerce, 32
+  %a.sroa.2.0.extract.trunc = trunc i64 %a.sroa.2.0.extract.shift to i32
+  %1 = bitcast i32 %a.sroa.2.0.extract.trunc to float
+  %b.sroa.0.0.extract.trunc = trunc i64 %b.coerce to i32
+  %2 = bitcast i32 %b.sroa.0.0.extract.trunc to float
+  %b.sroa.2.0.extract.shift = lshr i64 %b.coerce, 32
+  %b.sroa.2.0.extract.trunc = trunc i64 %b.sroa.2.0.extract.shift to i32
+  %3 = bitcast i32 %b.sroa.2.0.extract.trunc to float
+  %add.r = fadd float %0, %2
+  %add.i = fadd float %1, %3
+  %4 = bitcast float %add.r to i32
+  %5 = bitcast float %add.i to i32
+  %retval.sroa.2.0.insert.ext = zext i32 %5 to i64
+  %retval.sroa.2.0.insert.shift = shl nuw i64 %retval.sroa.2.0.insert.ext, 32
+  %retval.sroa.0.0.insert.ext = zext i32 %4 to i64
+  %retval.sroa.0.0.insert.insert = or i64 %retval.sroa.2.0.insert.shift, %retval.sroa.0.0.insert.ext
+  ret i64 %retval.sroa.0.0.insert.insert
+}