Page MenuHomePhabricator

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

Authored by shiva0217 on Tue, Jul 30, 10:44 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Tue, Jul 30, 10:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 30, 10:44 PM

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?

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.

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 updated this revision to Diff 212746.Thu, Aug 1, 1:16 AM
shiva0217 retitled this revision from [RISCV] Generate extensions for RV64 when lowering LibCall with i32 type to [RISCV] Avoid generating AssertZext for RV64 when lowering floating Libcall.
shiva0217 edited the summary of this revision. (Show Details)

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 added inline comments.Thu, Aug 1, 5:24 AM
test/CodeGen/RISCV/rv32i-rv64i-float-double.ll
37

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

shiva0217 marked an inline comment as done.Thu, Aug 1, 7:11 PM
shiva0217 added inline comments.
test/CodeGen/RISCV/rv32i-rv64i-float-double.ll
37

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

shiva0217 updated this revision to Diff 212952.Thu, Aug 1, 7:12 PM

Update the patch to catch divsf3 case.

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

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.

3032

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 marked an inline comment as done.Tue, Aug 6, 2:31 AM
shiva0217 added inline comments.
include/llvm/CodeGen/TargetLowering.h
3032

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 marked 2 inline comments as done.Wed, Aug 7, 1:06 AM
shiva0217 added inline comments.
include/llvm/CodeGen/TargetLowering.h
1834

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 updated this revision to Diff 213807.Wed, Aug 7, 1:17 AM
shiva0217 retitled this revision from [RISCV] Avoid generating AssertZext for RV64 when lowering floating Libcall to [RISCV] Avoid generating AssertZext for LP64 ABI when lowering floating Libcall.
shiva0217 edited the summary of this revision. (Show Details)

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.

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

unreachable?

test/CodeGen/RISCV/float-fcmp.ll
7 ↗(On Diff #213807)

Please commit the new RUN line separately.

shiva0217 updated this revision to Diff 214309.Thu, Aug 8, 11:48 PM
shiva0217 edited the summary of this revision. (Show Details)

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.

lenary edited reviewers, added: luismarques; removed: lenary.Mon, Aug 12, 7:53 AM
luismarques added inline comments.Thu, Aug 15, 2:05 PM
include/llvm/CodeGen/TargetLowering.h
3565

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
136

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

shiva0217 marked 2 inline comments as done.Mon, Aug 19, 7:35 PM
shiva0217 added inline comments.
include/llvm/CodeGen/TargetLowering.h
3565

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
136

Thanks for catching this. It seems much cleaner.

shiva0217 updated this revision to Diff 216042.Mon, Aug 19, 7:43 PM

Update patch to reflect the comments.

Hi @luismarques, thanks for the review.

efriedma added inline comments.Tue, Aug 20, 1:08 PM
include/llvm/CodeGen/TargetLowering.h
3565

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 updated this revision to Diff 216309.Tue, Aug 20, 6:40 PM

Add comment for the new field in MakeLibCallOptions struct.

Hi @efriedma, thanks for your guidance.