[RISCV] Remove duplicate WriteRes<WriteJalr for MIPSP8700.

[topperc]

We had two WriteRes for WriteJalr with difference latencies. Drop the duplicate and change the latency of Jal to 1.

[llvmbot]

@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

We had two WriteRes for WriteJalr with difference latencies. I don't know which is correct. I chose Latency=2 to match WriteJal.

---

Full diff: https://github.com/llvm/llvm-project/pull/123865.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVSchedMIPSP8700.td (-1)

diff --git a/llvm/lib/Target/RISCV/RISCVSchedMIPSP8700.td b/llvm/lib/Target/RISCV/RISCVSchedMIPSP8700.td
index 550f83a59b8b0e..ab6402dc96af10 100644
--- a/llvm/lib/Target/RISCV/RISCVSchedMIPSP8700.td
+++ b/llvm/lib/Target/RISCV/RISCVSchedMIPSP8700.td
@@ -125,7 +125,6 @@ def : WriteRes<WriteCSR, [p8700ALQ]>;
 
 // Handle CTI Pipeline.
 def : WriteRes<WriteJmp, [p8700IssueCTI]>;
-def : WriteRes<WriteJalr, [p8700IssueCTI]>;
 let Latency = 2 in {
 def : WriteRes<WriteJal, [p8700IssueCTI]>;
 def : WriteRes<WriteJalr, [p8700IssueCTI]>;

[wangpc-pp]

(Actually I think the latency of jal/jalr doesn't matter at all, since they are scheduling boundaries anyway.)

[topperc]

(Actually I think the latency of jal/jalr doesn't matter at all, since they are scheduling boundaries anyway.)

What about llvm-mca?

[wangpc-pp]

(Actually I think the latency of jal/jalr doesn't matter at all, since they are scheduling boundaries anyway.)

What about llvm-mca?

llvm-mca assumes the latency is 100 (can be specified via -call-latency option) for instructions with isCall = 1:

llvm-project/llvm/lib/MCA/InstrBuilder.cpp

Lines 222 to 237 in 7c58d63

	static void computeMaxLatency(InstrDesc &ID, const MCInstrDesc &MCDesc,
	const MCSchedClassDesc &SCDesc,
	const MCSubtargetInfo &STI,
	unsigned CallLatency) {
	if (MCDesc.isCall()) {
	// We cannot estimate how long this call will take.
	// Artificially set an arbitrarily high latency.
	ID.MaxLatency = CallLatency;
	return;
	}
	int Latency = MCSchedModel::computeInstrLatency(STI, SCDesc);
	// If latency is unknown, then conservatively assume the MaxLatency set for
	// calls.
	ID.MaxLatency = Latency < 0 ? CallLatency : static_cast<unsigned>(Latency);
	}

And I just found that we don't set isCall = 1 to JAL/JALR, is this by design?

[topperc]

(Actually I think the latency of jal/jalr doesn't matter at all, since they are scheduling boundaries anyway.)

What about llvm-mca?

llvm-mca assumes the latency is 100 (can be specified via -call-latency option) for instructions with isCall = 1:

llvm-project/llvm/lib/MCA/InstrBuilder.cpp

Lines 222 to 237 in 7c58d63

	static void computeMaxLatency(InstrDesc &ID, const MCInstrDesc &MCDesc,
	const MCSchedClassDesc &SCDesc,
	const MCSubtargetInfo &STI,
	unsigned CallLatency) {
	if (MCDesc.isCall()) {
	// We cannot estimate how long this call will take.
	// Artificially set an arbitrarily high latency.
	ID.MaxLatency = CallLatency;
	return;
	}
	int Latency = MCSchedModel::computeInstrLatency(STI, SCDesc);
	// If latency is unknown, then conservatively assume the MaxLatency set for
	// calls.
	ID.MaxLatency = Latency < 0 ? CallLatency : static_cast<unsigned>(Latency);
	}

And I just found that we don't set isCall = 1 to JAL/JALR, is this by design?

JAL/JALR isn't a "call" if the destination register is X0. So we have to analyze the operands. We have RISCVMCInstrAnalysis in the MC layer for this. Maybe llvm-mca should use that if it isn't?

[wangpc-pp]

We have RISCVMCInstrAnalysis in the MC layer for this. Maybe llvm-mca should use that if it isn't?

Yeah! I have done it in #123882.

[djtodoro]

Actually, both JAL and JALR have the Latency=1.

[mshockwave]

LGTM

[michaelmaitland]

Is this something we should enforce in TableGen -- no duplicates allowed?

[mshockwave]

Is this something we should enforce in TableGen -- no duplicates allowed?

does #123876 enforce this? Specifically when it's adding new WriteRes into the map.

[topperc]

Is this something we should enforce in TableGen -- no duplicates allowed?

does #123876 enforce this? Specifically when it's adding new WriteRes into the map.

Yes. It was enforce previously, but #92202 broke it. Without the early exit in the loop we would find two different WriteRes with the same WriteType.

#123876 adds back enforcement when we add to the map.