[AMDGPU] Prefer s_memtime for readcyclecounter on GFX10
#80211
Conversation
Summary: The old `s_memtime` instruction was supported until the GFX10 architecture. Although this instruction has a higher latency than the new shader counter, it's much more usable as a processor clock as it is a full 64-bit counter. The new shader counter is only a 20-bit counter, which makes it difficult to use as a standard cycle counter as it will overflow in a few milliseconds. This patch suggests preferring `s_memtime` for this instrinsic if it is still available.
jhuber6
requested review from
arsenm,
b-sumner,
jayfoad,
JonChesterfield,
rampitec and
shiltian
|
@llvm/pr-subscribers-backend-amdgpu Author: Joseph Huber (jhuber6) ChangesSummary: Full diff: https://github.com/llvm/llvm-project/pull/80211.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SMInstructions.td b/llvm/lib/Target/AMDGPU/SMInstructions.td
index f082de35b6ae9..f3096962e2f3e 100644
--- a/llvm/lib/Target/AMDGPU/SMInstructions.td
+++ b/llvm/lib/Target/AMDGPU/SMInstructions.td
@@ -1065,8 +1065,6 @@ def : GCNPat <
(REG_SEQUENCE SReg_64,
(S_GETREG_B32 getHwRegImm<HWREG.SHADER_CYCLES, 0, -12>.ret), sub0,
(S_MOV_B32 (i32 0)), sub1)> {
- // Prefer this to s_memtime because it has lower and more predictable latency.
- let AddedComplexity = 1;
}
} // let OtherPredicates = [HasShaderCyclesRegister]
diff --git a/llvm/test/CodeGen/AMDGPU/readcyclecounter.ll b/llvm/test/CodeGen/AMDGPU/readcyclecounter.ll
index 046a6d0a8cb7e..fd422b344d834 100644
--- a/llvm/test/CodeGen/AMDGPU/readcyclecounter.ll
+++ b/llvm/test/CodeGen/AMDGPU/readcyclecounter.ll
@@ -4,8 +4,8 @@
; RUN: llc -global-isel=1 -mtriple=amdgcn -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefix=MEMTIME -check-prefix=SIVI -check-prefix=GCN %s
; RUN: llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck -check-prefix=MEMTIME -check-prefix=GCN %s
; RUN: llc -global-isel=1 -mtriple=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck -check-prefix=MEMTIME -check-prefix=GCN %s
-; RUN: llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1030 -verify-machineinstrs < %s | FileCheck -check-prefixes=GETREG,GETREG-SDAG -check-prefix=GCN %s
-; RUN: llc -global-isel=1 -mtriple=amdgcn -mcpu=gfx1030 -verify-machineinstrs < %s | FileCheck -check-prefixes=GETREG,GETREG-GISEL -check-prefix=GCN %s
+; RUN: llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1030 -verify-machineinstrs < %s | FileCheck -check-prefixes=MEMTIME -check-prefix=GCN %s
+; RUN: llc -global-isel=1 -mtriple=amdgcn -mcpu=gfx1030 -verify-machineinstrs < %s | FileCheck -check-prefixes=MEMTIME -check-prefix=GCN %s
; RUN: llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1100 -verify-machineinstrs -amdgpu-enable-vopd=0 < %s | FileCheck -check-prefixes=GETREG,GETREG-SDAG -check-prefix=GCN %s
; RUN: llc -global-isel=1 -mtriple=amdgcn -mcpu=gfx1100 -verify-machineinstrs -amdgpu-enable-vopd=0 < %s | FileCheck -check-prefixes=GETREG,GETREG-GISEL -check-prefix=GCN %s
; RUN: llc -global-isel=0 -mtriple=amdgcn -mcpu=gfx1200 -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN,GFX12 %s
|
|
Umm...the cycle counter on gfx11 is 20 bits wide. I would oppose an attempt to implement the cycle counter with the realtime counter. |
gfx10 still has this 64-bit version, but there's no alternative on gfx11. I don't know if gfx12 remedied this issue or not. Are you proposing gfx10 follows what gfx11 uses even when we have access to a higher resolution clock? |
Sorry, I'd completely forgotten about that part of gfx10. I agree that s_memtime should be used for readcyclecounter. |
| // Prefer this to s_memtime because it has lower and more predictable latency. | ||
| let AddedComplexity = 1; |
There was a problem hiding this comment.
Is the whole pattern just dead now? I would expect to need either an extra predicate for is-useful or modify the HasShaderCyclesRegister predicate
There was a problem hiding this comment.
I'm not intimately familiar with the backend. My understanding is that the added complexity ensures SelectionDAG will choose this lowering instead of the s_memtime one in cases where both are available. The only platform where both are available is gfx10 AFAIK, so removing this makes it work on gfx10. Would we need some more specific predicates? My assumption was that no new cards would have s_memtime and the shader counter at the same time.
Summary: The old `s_memtime` instruction was supported until the GFX10 architecture. Although this instruction has a higher latency than the new shader counter, it's much more usable as a processor clock as it is a full 64-bit counter. The new shader counter is only a 20-bit counter, which makes it difficult to use as a standard cycle counter as it will overflow in a few milliseconds. This patch suggests preferring `s_memtime` for this instrinsic if it is still available.
Summary: The old `s_memtime` instruction was supported until the GFX10 architecture. Although this instruction has a higher latency than the new shader counter, it's much more usable as a processor clock as it is a full 64-bit counter. The new shader counter is only a 20-bit counter, which makes it difficult to use as a standard cycle counter as it will overflow in a few milliseconds. This patch suggests preferring `s_memtime` for this instrinsic if it is still available.
Summary: The old `s_memtime` instruction was supported until the GFX10 architecture. Although this instruction has a higher latency than the new shader counter, it's much more usable as a processor clock as it is a full 64-bit counter. The new shader counter is only a 20-bit counter, which makes it difficult to use as a standard cycle counter as it will overflow in a few milliseconds. This patch suggests preferring `s_memtime` for this instrinsic if it is still available.
Summary:
The old
s_memtimeinstruction was supported until the GFX10architecture. Although this instruction has a higher latency than the
new shader counter, it's much more usable as a processor clock as it is
a full 64-bit counter. The new shader counter is only a 20-bit counter,
which makes it difficult to use as a standard cycle counter as it will
overflow in a few milliseconds. This patch suggests preferring
s_memtimefor this instrinsic if it is still available.