Page MenuHomePhabricator

[RISCV GlobalISel] Add lowerReturn for calling conv.
Needs ReviewPublic

Authored by wwei on Nov 4 2019, 9:46 AM.

Details

Summary

Add minimal support to lower return, and introduce an outgoing ValueHandler for returns.
Support return values with integer, pointer and aggregate types.

Diff Detail

Event Timeline

wwei created this revision.Nov 4 2019, 9:46 AM
lewis-revill added inline comments.Nov 19 2019, 10:25 AM
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;
lewis-revill added inline comments.Dec 17 2019, 8:53 AM
llvm/lib/Target/RISCV/RISCVCallLowering.cpp
55

What's the case where this will be used?

llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll
0–1

Nitpick: no longer true with these additions.

wwei marked an inline comment as done.Dec 18 2019, 8:43 AM
wwei added inline comments.
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll
83

Yes, it's a typo, I'll fix it.

wwei marked an inline comment as done.Dec 18 2019, 8:57 AM
wwei added inline comments.
llvm/lib/Target/RISCV/RISCVCallLowering.cpp
114

To make the code look clearer and simpler, I will encapsulate a function to handle this condition.

wwei updated this revision to Diff 234556.Dec 18 2019, 9:05 AM

Fix some typos and add a new function lowerReturnVal

wwei marked 2 inline comments as done.Dec 18 2019, 9:16 AM
wwei added inline comments.
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.

lewis-revill added inline comments.Dec 19 2019, 6:20 AM
llvm/lib/Target/RISCV/RISCVCallLowering.cpp
101

Nitpick: add the /*IsRet=*/ comment.
TLI.analyzeOutputArgs(MF, CCInfo, Outs, /*IsRet=*/true, nullptr);

wwei updated this revision to Diff 241479.Jan 30 2020, 8:43 AM
wwei marked an inline comment as done.
wwei added inline comments.
llvm/lib/Target/RISCV/RISCVCallLowering.cpp
101

done

wwei marked an inline comment as done.Jan 30 2020, 8:45 AM
wwei added inline comments.
llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll
0–1

removed.

wwei marked an inline comment as done.Jan 30 2020, 8:46 AM
wwei added inline comments.
llvm/lib/Target/RISCV/RISCVCallLowering.cpp
55

That's correct.

wwei marked an inline comment as done.Jan 30 2020, 8:58 AM
wwei added inline comments.
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.

Joe added a subscriber: Joe.Mar 10 2020, 8:05 AM
dsanders added inline comments.Mar 19 2020, 10:54 AM
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