Add minimal support to lower return, and introduce an outgoing ValueHandler for returns.
Support return values with integer, pointer and aggregate types.
Details
Diff Detail
Event Timeline
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll | ||
---|---|---|
83 | Typo: the pattern here should be [[AEXT]], right? |
See comments above. Otherwise, this looks good to me although others should evaluate too.
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
114 | I don't know whether other people prefer this as well, but I'd prefer the condition here to be reversed and simply duplicate the MIRBuilder.insertInstr(Ret); return true; IE: if (Val == nullptr) { MIRBuilder.insertInstr(Ret); return true; } // ... MIRBuilder.insertInstr(Ret); return true; |
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll | ||
---|---|---|
83 | Yes, it's a typo, I'll fix it. |
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
114 | To make the code look clearer and simpler, I will encapsulate a function to handle this condition. |
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
55 | This is a reserved case. Currently, riscv backend does not implement CCAssignFnForCall and CCAssignFnForReturn , so AssignFn will be always null. If these two functions will be added later, the code is available directly. | |
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll | ||
0–1 | I don't understand why many test files add this assertion. So is your suggestion that we can remove this assertion? |
Thank you for the update! Please see comments.
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
55 | Correct me if I'm wrong but won't the assignments already have been done when you call TLI.analyzeOutputArgs from lowerReturn? IE: TLI.analyzeOutputArgs currently calls CC_RISCV, which essentially performs the task of a CCAssignFn. | |
114 | That's not quite what I meant by that, though I understand the desire to reduce code duplication. Personally I prefer a little bit of duplication of code like I suggested instead of adding another helper function. | |
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll | ||
0–1 | Yes the comment can be removed. This is added when the test file CHECK annotations are automatically added using that tool. |
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
101 | Nitpick: add the /*IsRet=*/ comment. |
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
101 | done |
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll | ||
---|---|---|
0–1 | removed. |
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
55 | That's correct. |
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
114 | In my opinion, using a helper function will make the code more clear and concise, which is really a personal preference issue, regardless of whether the implementation is correct or not. |
Thanks for addressing the comments, I'd like some input from others before saying this looks good.
llvm/lib/Target/RISCV/RISCVCallLowering.cpp | ||
---|---|---|
159 | You shouldn't need to drop all the way down to align 1 for the subsequent parts. For example if we start with 8 byte alignment, an offset of 4 leaves us with 4 bytes alignment. commonAlignment(Align A, uint64_t Offset) should compute it for you |
What's the case where this will be used?