[RISCV] Add sched to pseudo function call instructions

[• pcwang-thead]

Summary

To fix llvm-mca's error of 'found an unsupported instruction
in the input assembly sequence.' caused by the lack of
scheduling info.

Pseudo function call instructions will be expanded to auipc
and jalr, so their scheduling info are the combination of
two.

This may not be the right way to fix the error. I have tried
to expand calls in RISCVAsmParser but it failed because of
some encoding errors.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

Don't exceed 80 characters per line.

1282–1283

I'm not sure it's correct to have more than one Write scheduling class with only a single output register. Each Write is supposed to correspond to the latency of the register.

[• pcwang-thead]

Format code.

[• pcwang-thead]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

According to comments of Sched class in llvm/include/llvm/Target/TargetSchedule.td:

One SchedWrite type must be listed for each explicit def operand in order. Additional SchedWrite types may optionally be listed for implicit def operands.

It is legal to add more Writes for implicit operands and I see a lot of definitions like this in other targets. Example in AArch64:

def TLSDESC_CALLSEQ
    : Pseudo<(outs), (ins i64imm:$sym),
             [(AArch64tlsdesc_callseq tglobaltlsaddr:$sym)]>,
      Sched<[WriteI, WriteLD, WriteI, WriteBrReg]>;

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

My understanding of "implicit def operands" would be physical registers listed in let Defs = [] syntax.

@andreadb is this the right thing to do?

[• pcwang-thead]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

I'm not for sure. Here is an example in llvm/lib/Target/AArch64/AArch64InstrInfo.td:

let isCall = 1, Defs = [LR, X0, X1], hasSideEffects = 1, Size = 16,
    isCodeGenOnly = 1 in
def TLSDESC_CALLSEQ
    : Pseudo<(outs), (ins i64imm:$sym),
             [(AArch64tlsdesc_callseq tglobaltlsaddr:$sym)]>,
      Sched<[WriteI, WriteLD, WriteI, WriteBrReg]>;

Defs are 3 registers ([LR, X0, X1]) and there is no output register, while there are 4 writes.

[andreadb]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

Sorry for the late reply (I am just back from holiday).

My understanding of "implicit def operands" would be physical registers listed in let Defs = [] syntax.

@andreadb is this the right thing to do?

Your understanding is correct.

Each write is associated with a specific register definition. The sequence of writes matters, and implicit definitions always follow explicit definitions. Implicit definitions come from the let Defs = [...] directive.

To answer to your question:

It is legal to declare a scheduling class with more writes than register definitions.
The subtarget emitter doesn't consider it a user error, and a valid scheduling class would be generated for it.

The generated scheduling class would declare a set of MCWriteProcResEntry entries which also includes contributions from the extra write(s). However, that class would declare more MCWriteLatencyEntry than register definitions.

Scheduling algorithms would still work fine because method MCSchedModel::computeInstrLatency always iterates over the fill set of MCWriteLatencyEntry when computing the "max" instruction latency.

Method TargetSchedModel::computeOperandLatency (lib/CodeGen/TargetSchedule.cpp) can be used by scheduling algorithms to query the latency of specific register definition. That method would still work fine (simply because it would never be queried for an invalid (e.g. off-by-one) DefOperIdx value.

In conclusion: resource consumption related to extra writes would be added to the set of processor resources. Extra writes can still affect the total instruction latency. However, scheduling algorithms won't be able to associate those to any particular register operand.

Ideally, if possible, it would be better to have a matching number of writes. That's just my opinion though.

The TLSDESC_CALLSEQ pseudo that Wang mentioned in his last comment is associated with a scheduling class descriptor named WriteI_WriteLD_WriteI_WriteBrReg (whose definition can be found in AArch64GenSubtargetInfo.inc).
That scheduling class reports the right number of MCWriteProcResEntry (it also includes contributions from the "extra" write, i.e. WriteBrReg).
The "problem" is that it declares 4 MCWriteLatencyEntry, but the instruction only specifies 3 (implicit) register writes.
So, the fourth write latency cannot be attributed to any register operand in particular.

I hope it helps.

[• pcwang-thead]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

Thank you for your detailed explanation! I have learnt a lot from it!

So such definitions in this patch and other targets won't cause errors and scheduler will work fine, the only 'problem' is that we can't associate extra writes to any specific register. But I think it is not a problem in this patch, because all calls are scheduling boundaries (sees isSchedBoundary in CodeGen/MachineScheduler.cpp) and llvm-mca simply sets max latency of calls to 100 cycles.

Correct me if I am wrong, this patch makes no difference to scheduler, just makes llvm-mca happy. :)

[andreadb]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1282–1283

Thank you for your detailed explanation! I have learnt a lot from it!

So such definitions in this patch and other targets won't cause errors and scheduler will work fine, the only 'problem' is that we can't associate extra writes to any specific register. But I think it is not a problem in this patch, because all calls are scheduling boundaries (sees isSchedBoundary in CodeGen/MachineScheduler.cpp) and llvm-mca simply sets max latency of calls to 100 cycles.

Correct me if I am wrong, this patch makes no difference to scheduler, just makes llvm-mca happy. :)

It makes no difference to scheduling algorithms because - as you wrote - calls are treated like scheduling boundaries anyway.

I was a bit surprised to see that this patch is required for llvm-mca.

Is it because the RISCV assembly parser can match assembly against a "Pseudo" MCInst? For example, does this means that call foo is parsed as MCInst PseudoCALL $foo ?

If so, then the change looks good to me.

[craig.topper]

LGTM

[zixuan-wu]

I think using two Write is not accurate because the latency are not added together but is the max one. It consumes resource simultaneously, but the expanded instructions auipc and jalr do have data dependency.
I think it should create a new SchedWrite such as WritePesudoCall and the cycle is sum up of WriteIALU and WriteJalr.

let Latency = 2 in
def : WriteRes<WritePesudoCall, [RocketUnitALU, RocketUnitB]>;

Or we can make it simple with leveraging WriteSequence to create WritePesudoCall.

def WritePesudoCall : WriteSequence<[WriteIALU, WriteJalr]>;

[• pcwang-thead]

In D123578#3470352, @zixuan-wu wrote:

I think using two Write is not accurate because the latency are not added together but is the max one. It consumes resource simultaneously, but the expanded instructions auipc and jalr do have data dependency.
I think it should create a new SchedWrite such as WritePesudoCall and the cycle is sum up of WriteIALU and WriteJalr.

let Latency = 2 in
def : WriteRes<WritePesudoCall, [RocketUnitALU, RocketUnitB]>;

Or we can make it simple with leveraging WriteSequence to create WritePesudoCall.

def WritePesudoCall : WriteSequence<[WriteIALU, WriteJalr]>;

I think it is unnecessary, since calls won't be part of scheduling regions.
As we discussed above, this patch just wants to make llvm-mca happy. :)

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 
 // PseudoCALLReg is a generic pseudo instruction for calls which will eventually
 // expand to auipc and jalr while encoding, with any given register used as the
 // destination.
 // Define AsmString to print "call" when compile with -S flag.
 // Define isCodeGenOnly = 0 to support parsing assembly "call" instruction.
 let isCall = 1, isBarrier = 1, isCodeGenOnly = 0, Size = 8, hasSideEffects = 0,
     mayStore = 0, mayLoad = 0 in
-def PseudoCALLReg : Pseudo<(outs GPR:$rd), (ins call_symbol:$func), []> {
+def PseudoCALLReg : Pseudo<(outs GPR:$rd), (ins call_symbol:$func), []>,
+                    Sched<[WriteIALU, WriteJalr, ReadJalr]> {
   let AsmString = "call\t$rd, $func";
 }
 
 // PseudoCALL is a pseudo instruction which will eventually expand to auipc
 // and jalr while encoding. This is desirable, as an auipc+jalr pair with
 // R_RISCV_CALL and R_RISCV_RELAX relocations can be be relaxed by the linker
 // if the offset fits in a signed 21-bit immediate.
 // Define AsmString to print "call" when compile with -S flag.
 // Define isCodeGenOnly = 0 to support parsing assembly "call" instruction.
 let isCall = 1, Defs = [X1], isCodeGenOnly = 0, Size = 8 in
-def PseudoCALL : Pseudo<(outs), (ins call_symbol:$func), []> {
+def PseudoCALL : Pseudo<(outs), (ins call_symbol:$func), []>,
+                 Sched<[WriteIALU, WriteJalr, ReadJalr]> {
   let AsmString = "call\t$func";
 }
 
 def : Pat<(riscv_call tglobaladdr:$func), (PseudoCALL tglobaladdr:$func)>;
 def : Pat<(riscv_call texternalsym:$func), (PseudoCALL texternalsym:$func)>;
 
 def : Pat<(riscv_uret_flag), (URET X0, X0)>;
 def : Pat<(riscv_sret_flag), (SRET X0, X0)>;
 def : Pat<(riscv_mret_flag), (MRET X0, X0)>;
 
 let isCall = 1, Defs = [X1] in
 def PseudoCALLIndirect : Pseudo<(outs), (ins GPRJALR:$rs1),
                                 [(riscv_call GPRJALR:$rs1)]>,
                          PseudoInstExpansion<(JALR X1, GPR:$rs1, 0)>;
 
 let isBarrier = 1, isReturn = 1, isTerminator = 1 in
 def PseudoRET : Pseudo<(outs), (ins), [(riscv_ret_flag)]>,
                 PseudoInstExpansion<(JALR X0, X1, 0)>;
 
 // PseudoTAIL is a pseudo instruction similar to PseudoCALL and will eventually
 // expand to auipc and jalr while encoding.
 // Define AsmString to print "tail" when compile with -S flag.
 let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [X2],
     Size = 8, isCodeGenOnly = 0 in
-def PseudoTAIL : Pseudo<(outs), (ins call_symbol:$dst), []> {
+def PseudoTAIL : Pseudo<(outs), (ins call_symbol:$dst), []>,
+                 Sched<[WriteIALU, WriteJalr, ReadJalr]> {
   let AsmString = "tail\t$dst";
 }
 
 let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [X2] in
 def PseudoTAILIndirect : Pseudo<(outs), (ins GPRTC:$rs1),
                                 [(riscv_tail GPRTC:$rs1)]>,
                          PseudoInstExpansion<(JALR X0, GPR:$rs1, 0)>;
 
 def : Pat<(riscv_tail (iPTR tglobaladdr:$dst)),
           (PseudoTAIL texternalsym:$dst)>;
 def : Pat<(riscv_tail (iPTR texternalsym:$dst)),
           (PseudoTAIL texternalsym:$dst)>;
 
 let isCall = 0, isBarrier = 1, isBranch = 1, isTerminator = 1, Size = 8,
     isCodeGenOnly = 0, hasSideEffects = 0, mayStore = 0, mayLoad = 0 in
-def PseudoJump : Pseudo<(outs GPR:$rd), (ins pseudo_jump_symbol:$target), []> {
+def PseudoJump : Pseudo<(outs GPR:$rd), (ins pseudo_jump_symbol:$target), []>,
+                 Sched<[WriteIALU, WriteJalr, ReadJalr]> {
   let AsmString = "jump\t$target, $rd";
 }
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0, Size = 8, isCodeGenOnly = 0,
     isAsmParserOnly = 1 in
 def PseudoLLA : Pseudo<(outs GPR:$dst), (ins bare_symbol:$src), [],
                        "lla", "$dst, $src">;