This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][GlobalISel] Add lowerReturn for calling conv
ClosedPublic

Authored by nitinjohnraj on Jan 14 2022, 7:26 AM.

Details

Summary

Add minimal support to lower return, and introduce an OutgoingValueHandler and an OutgoingValueAssigner for returns.

Supports return values with integer, pointer and aggregate types.

(Update of D69808 - avoiding commandeering that revision)

Diff Detail

Event Timeline

lewis-revill created this revision.Jan 14 2022, 7:26 AM
lewis-revill requested review of this revision.Jan 14 2022, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 7:26 AM
arsenm added inline comments.Jan 17 2022, 4:17 PM
llvm/lib/CodeGen/TargetRegisterInfo.cpp
237–239 ↗(On Diff #399993)

This needs its own patch and justification

llvm/lib/Target/RISCV/RISCVCallLowering.cpp
112–116

This looks like you're copying the broken form of splitToValueTypes from x86. I think trying to handle all of the argument marshalling in simple looking callbacks like this is unworkable. If you call the generic splitToValueTypes in CallLowering, it should try to handle everything for you

148–175

This is reinventing generic infrastructure

177

Ditto, this is a less complete copy of the generic infrastructure

219

Ditto

lewis-revill edited the summary of this revision. (Show Details)

Don't reinvent existing generic functions; instead introduce an OutgoingValueAssigner to allow us to call determineAndHandleAssignments.

arsenm accepted this revision.Jan 26 2022, 4:28 PM
This revision is now accepted and ready to land.Jan 26 2022, 4:28 PM
lewis-revill retitled this revision from [WIP][RISCV][GlobalISel] Add lowerReturn for calling conv to [RISCV][GlobalISel] Add lowerReturn for calling conv.Feb 9 2022, 3:14 AM
arsenm added a comment.Feb 9 2022, 8:26 AM

LGTM

llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/ret.ll
144

Probably should have some vectors

nitinjohnraj commandeered this revision.Apr 5 2023, 12:59 PM
nitinjohnraj added a reviewer: lewis-revill.
0x59616e removed a subscriber: 0x59616e.Apr 5 2023, 7:43 PM
eopXD added inline comments.Apr 12 2023, 5:10 AM
llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
83 ↗(On Diff #511190)

This looks redundant.

llvm/lib/Target/RISCV/RISCVISelLowering.h
564

Can we avoid this change?

668

Why do we need to promote the static functions into the header here?

nitinjohnraj marked an inline comment as done.

Addressing comments

nitinjohnraj requested review of this revision.May 22 2023, 8:39 AM

This has been stagnant for a while, and there were a few open comments, so could I reaffirm the approval of this patch?

llvm/lib/Target/RISCV/RISCVISelLowering.h
564

This change is in order to use RISCVCCAssignFn in RISCVOutgoingValueAssigner above. We could maybe avoid that by inlining the type instead of changing the access specifier, but should we?

668

In order to use them on line 111 in llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp. Should we maybe move this outside the RISCV namespace?

llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/ret.ll
144

If I understand correctly, this patch doesn't handle vectors.

arsenm added inline comments.May 22 2023, 8:49 AM
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/ret.ll
103–104

New tests must use opaque pointers

Used opaque pointer in test

nitinjohnraj marked an inline comment as done.May 22 2023, 11:45 AM
nitinjohnraj marked 2 inline comments as not done.
craig.topper added inline comments.May 22 2023, 11:58 AM
llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
123 ↗(On Diff #524411)

Drop curly braces

llvm/lib/Target/RISCV/GISel/RISCVCallLowering.h
22 ↗(On Diff #524411)

alphabetize the forward declarations

arsenm accepted this revision.May 22 2023, 1:17 PM
This revision is now accepted and ready to land.May 22 2023, 1:17 PM
evandro removed a subscriber: evandro.May 22 2023, 5:24 PM

Addressed comments

Added lewis as co-author in commit message

This revision was landed with ongoing or failed builds.May 23 2023, 11:29 AM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp