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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

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 :)



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:88
 
 // Expand PseudoCALL to AUIPC and JALR with relocation types.
 // We expand PseudoCALL while encoding, meaning AUIPC and JALR won't go through
----------------
Should expand doc comment to explain it also expands PseudoTAIL and that it returns the number of instructions that are produced.

That said, I expect we're going to drop the PseudoTAILIndirect expansion from here and so there's less need to return the number of instructions inserted.


https://reviews.llvm.org/D45395