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
80–84 ↗(On Diff #399993)

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

132–159 ↗(On Diff #399993)

This is reinventing generic infrastructure

161 ↗(On Diff #399993)

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

203 ↗(On Diff #399993)

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
145

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

This looks redundant.

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

Can we avoid this change?

837

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
710

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?

837

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
145

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
124

Drop curly braces

llvm/lib/Target/RISCV/GISel/RISCVCallLowering.h
22–24

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.