[PATCH] D45395: [RISCV] Lower the tail pseudoinstruction

[Alex Bradbury via Phabricator via llvm-commits]

asb requested changes to this revision.
asb added a comment.
This revision now requires changes to proceed.

Hi Mandeep, I've added a number of inline comments and queries. I think we're getting pretty close now.

There's no test coverage for the patterns:

  def : Pat<(Tail (iPTR texternalsym:$dst)),
            (PseudoTAIL texternalsym:$dst)>;
  def : Pat<(Tail GPRTC:$dst),               
            (PseudoTAILIndirect GPRTC:$dst)>;



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1191-1192
+  else if (CLI.CS && CLI.CS.isMustTailCall())
+    report_fatal_error("failed to perform tail call elimination on a call "
+                       "site marked musttail");
+
----------------
Could you please add a test case for this, demonstrating that we get an error rather than silently miscompiling or ignoring musttail.  A musttail function with structret or similar should do the trick.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1313
 
-      // Emit the store.
-      MemOpChains.push_back(
-          DAG.getStore(Chain, DL, ArgValue, Address, MachinePointerInfo()));
+      if (!IsTailCall) {
+        SDValue Address =
----------------
We know that IsTailCall should never be true here. I'd get rid of this conditional and add an assert at line 1308. It seems better than introducing a control flow path we can't test and we know should never execute.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1331-1339
+  // Tail call byval lowering might overwrite argument registers so in case of
+  // tail call optimization the copies to registers are lowered to 'real' stack
+  // slot.
+  // Force all the incoming stack arguments to be loaded from the stack
+  // before any new outgoing arguments are stored to the stack, because the
+  // outgoing stack slots may alias the incoming argument stack slots, and
+  // the alias isn't otherwise explicit. This is slightly more conservative
----------------
Is this comment and logic actually relevant for the RISC-V implementation? In this patch, tail call optimisation is disabled for byval parameters and any case where the stack is used for parameter passing.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:672
 
+let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [X2] in
+def PseudoTAILIndirect : Pseudo<(outs), (ins GPRTC:$dst), []>;
----------------
Is Uses = [X2] required here?


================
Comment at: test/CodeGen/RISCV/disable-tail-calls.ll:1-2
+; Check that command line option "-disable-tail-calls" overrides function
+; attribute "disable-tail-calls".
+
----------------
The test is currently just testing whether -disable-tail-calls=false overrides a disable-tail-calls=true attribute. It should also test whether -disable-tail-calls=true overrides an explicit disable-tail-calls=false attribute.

The overriding behaviour seems a little surprising to me, but I see ARM is testing the same thing.


================
Comment at: test/CodeGen/RISCV/tail-calls.ll:1
+; RUN: llc -mtriple riscv32-unknown-linux-gnu -o - %s | FileCheck %s
+; RUN: llc -mtriple riscv32-unknown-elf       -o - %s | FileCheck %s
----------------
mgrang wrote:
> Note: I have limited this test only to riscv32. With riscv64 I get the following error for fp128 parameter:
> LLVM ERROR: Cannot select: t17: i64 = Constant<4611404543450677248>
> 
> I guess I can restrict only the failing test to riscv32 but I felt it was cleaner this way. We could enable this for riscv64 once the support for fp128 in riscv64 is more mature.
> 
> @asb What do you suggest?
MC layer tests should check rv64 whenever relevant. Proper RV64 codegen isn't yet upstreamed (primarily as I need to sit down and flesh out the tests). Just testing RV32 for now is the right thing to do


https://reviews.llvm.org/D45395