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

[Mandeep Singh Grang via Phabricator via llvm-commits]

mgrang added a comment.

In https://reviews.llvm.org/D45395#1102983, @asb wrote:

> I'm definitely seeing failures with this due to PseudoTAILIndirect.
>
> Sorry for being cryptic regarding handling of PseudoTAILIndirect. I meant it should have no asmstring because it should never reach the asm printer. It perhaps would be nice if there was an assert if an isCodeGenOnly instruction makes it through to the asm printer.
>
> PseudoTAILIndirect is purely a codegen construct, and won't produce the `tail` pseudoinstruction.  My first instinct would be to handle it very similarly to PseudoCALLIndirect and use PseudoInstExpansion:
>
>   def PseudoTAILIndirect : Pseudo<(outs), (ins GPRTC:$rs1), [(Tail GPRTC:$rs1)]>,
>                            PseudoInstExpansion<(JALR X0, GPR:$rs1, 0)>; 
>
>
> But I see there's some sort of issue there and haven't had a chance yet to dig deep into what's going on. Setting `let usesCustomInserter` and handling in EmitInstrWithCustomerInserter is an alternative approach that comes to mind, though it would be good to better understand the PseudoInstExpansion issue first.
>
> Test-wise, we definitely need coverage of PseudoInstExpansion. Beyond that, this seems like a nice optimisation to have. You mentioned it made not much difference in the workloads you threw at it, but it seems to trigger quite frequently in the sort of small test programs you find in a compiler test suite. If nothing else, it helps reduce the amount of code to look through when debugging those sort of inputs :)


Thanks @asb for the suggestions.
I marked PseudoTAILIndirect as usesCustomInserter and custom lowered it in EmitInstrWithCustomInserter: https://reviews.llvm.org/differential/diff/147624

However, I see that the stack is not restored before the tail call. The call parameters are also not setup correctly without writing code to carry over the implicit regs info from the Pseudo to the JALR. As a result, I see failures on some of the spec benchmarks (gap, gcc, hmmer, mesa). I tried to play around to see how we can setup the call but could not find a clean way.

On the other hand, I looked at how AArch64 lowers the TCRETURNri. They custom lower it in the AArch64AsmPrinter after the auto-generated pseudos are lowered.
I tried the same for RISCV and it seemed to work very well: https://reviews.llvm.org/differential/diff/147623

I see the tail call being setup correctly and the spec benchmarks are all passing. This way we also don't need any explicit asmstring in the td for the Pseudo. I think we should follow this route for lowering the PseudoTAILIndirect.
What do you think Alex ?


https://reviews.llvm.org/D45395