[PATCH] D69808: [RISCV GlobalISel] Add lowerReturn for calling conv.

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added a comment.

Thank you for the update! Please see comments.



================
Comment at: llvm/lib/Target/RISCV/RISCVCallLowering.cpp:55
+    if (AssignFn)
+      return AssignFn(ValNo, ValVT, LocVT, LocInfo, Flags, State);
+    return false;
----------------
wwei wrote:
> lewis-revill wrote:
> > What's the case where this will be used? 
> 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.
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`.


================
Comment at: llvm/lib/Target/RISCV/RISCVCallLowering.cpp:72
 
   if (Val != nullptr) {
+    // TODO: Only integer, pointer and aggregate types are supported now.
----------------
wwei wrote:
> lewis-revill wrote:
> > 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;
> > ```
> To make the code look clearer and simpler, I will encapsulate a function to handle this condition.
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.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/irtranslator-calllowering.ll:1
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=riscv32 -global-isel -stop-after=irtranslator -verify-machineinstrs < %s \
----------------
wwei wrote:
> lewis-revill wrote:
> > Nitpick: no longer true with these additions.
> I don't understand why many test files add this assertion. So is your suggestion that we can remove this assertion?
Yes the comment can be removed. This is added when the test file `CHECK` annotations are automatically added using that tool.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69808/new/

https://reviews.llvm.org/D69808