[PATCH] D45395: [RISCV] Implement tail call optimization

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Could you please separate the MC layer change into a separate patch?

It looks like there are cases where the stack is used for parameter passing and isEligibleForTailCallOptimization returns true, but there is no test coverage for these cases. Do you intend to enable tailcall optimisation for these cases? Might it be worth starting off by just enabling it for calls that make no use of the stack for parameter passing, thus simplifying the patch and the testing story?



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1029-1033
+  // RISCV has 8 function argument registers: [x10-x11], [x12-x17].
+  // If a function requires more argument registers we cannot tail call
+  // optimize it.
+  if (Outs.size() >= 8)
+    return false;
----------------
Handling ABI lowering for RV32D is rather fiddly, and the code that handles this breaks the assumption that one entry in Outs == one register (we use an approach similar to Mips, introducing buildpairf64 and splitf64 nodes to help workaround the fact that legalising and splitting and f64 to two i32 at ABI lowering time is _really_ fiddly when i64 isn't a legal type).

I think the safest way of checking eligibility is to pass a CCState to this function. If CCInfo.getNextStackOffset() == 0 then we know the stack isn't used.

Even without the RV32D case, you might have large values (e.g. i128 on RV32) that are passed via the stack. I'm assuming the intention is to not to tailcall if the stack is used at all, in which case getNextStackoffset is the better test.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1038-1039
+  // probably break this.
+  if (CallerF.hasFnAttribute("interrupt"))
+    return false;
+
----------------
The "interrupt" attribute isn't currently defined by RISC-V. At a minimum, it would be worth adding a note that this should be expanded as new function attributes are introduced.

I assume it would be too restrictive to have a whitelist of allowable attributes rather than a blacklist of attributes that disable tailcalling?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1043
+  // return semantics.
+  if (IsCalleeStructRet || CallerF.hasStructRetAttr())
+    return false;
----------------
Very minor, but it seems inconsistent that IsCalleeStructRet is calculate in the LowerCall but other properties such as CallerF.hasStructRetAttr or the presence of byval attributes are calculated here. I'd get rid of the IsCalleeStructRet argument.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:1114
+    // For tail calls, memory operands are available in our caller's stack.
+    NumBytes = 0;
+    ++NumTailCalls;
----------------
Also see my query in isEligibleForTailOptimization. If the intent is that tail calls aren't used when the stack is used, NumBytes should always be zero.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:663-672
+let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1,
+    isCodeGenOnly = 0, Uses = [X2] in {
+def PseudoTCALLdi : Pseudo<(outs), (ins bare_symbol:$dst), []> {
+  let AsmString = "tailcall\t$dst";
+}
+
+def PseudoTCALLri : Pseudo<(outs), (ins GPRTC:$dst), []> {
----------------
The correct name for this pseudoinstruction is "tail" (as used by gas and described in the ISA manual).


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:665
+    isCodeGenOnly = 0, Uses = [X2] in {
+def PseudoTCALLdi : Pseudo<(outs), (ins bare_symbol:$dst), []> {
+  let AsmString = "tailcall\t$dst";
----------------
PseudoTAIL would make sense, and matches then naming convention for PseudoCALL/PseudoCALLIndirect.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:669-671
+def PseudoTCALLri : Pseudo<(outs), (ins GPRTC:$dst), []> {
+  let AsmString = "tailcall\t$dst";
+}
----------------
This is going to have problems as currently defined. It assumes that you can specify `tail $reg` and have it work. As we have to always interpret an argument to tail as a symbol, this is going to lead to either incorrectly linked code or an error about an undefined symbol. Defining PseudoTAILIndirect with no AsmString might be a viable option?


https://reviews.llvm.org/D45395