Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Rework IDiv and FDiv pipes on SiFive7 #73970

Merged
merged 4 commits into from
Dec 4, 2023

Conversation

michaelmaitland
Copy link
Contributor

Set BufferSize=0 and remove Super pipes for these resources.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 30, 2023

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

Author: Michael Maitland (michaelmaitland)

Changes

Set BufferSize=0 and remove Super pipes for these resources.


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

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVSchedSiFive7.td (+2-2)
diff --git a/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td b/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
index 53ef9d1baf7b59a..261c22ea35317e2 100644
--- a/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
+++ b/llvm/lib/Target/RISCV/RISCVSchedSiFive7.td
@@ -213,12 +213,12 @@ let SchedModel = SiFive7Model in {
 let BufferSize = 0 in {
 def SiFive7PipeA       : ProcResource<1>;
 def SiFive7PipeB       : ProcResource<1>;
+def SiFive7IDiv        : ProcResource<1> { let Super = SiFive7PipeB; } // Int Division
+def SiFive7FDiv        : ProcResource<1> { let Super = SiFive7PipeB; } // FP Division/Sqrt
 def SiFive7PipeV       : ProcResource<1>;
 }
 
 let BufferSize = 1 in {
-def SiFive7IDiv        : ProcResource<1> { let Super = SiFive7PipeB; } // Int Division
-def SiFive7FDiv        : ProcResource<1> { let Super = SiFive7PipeB; } // FP Division/Sqrt
 def SiFive7VA          : ProcResource<1> { let Super = SiFive7PipeV; } // Arithmetic sequencer
 def SiFive7VL          : ProcResource<1> { let Super = SiFive7PipeV; } // Load sequencer
 def SiFive7VS          : ProcResource<1> { let Super = SiFive7PipeV; } // Store sequencer

@@ -213,12 +213,12 @@ let SchedModel = SiFive7Model in {
let BufferSize = 0 in {
def SiFive7PipeA : ProcResource<1>;
def SiFive7PipeB : ProcResource<1>;
def SiFive7IDiv : ProcResource<1> { let Super = SiFive7PipeB; } // Int Division
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why these need a Super class. Their usages already say SiFive7PipeB in the ResourceCycles explicitly.

Copy link
Contributor

@wangpc-pp wangpc-pp Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the bahavior is different. If there is no Super, SiFive7PipeB will be released after 1 cycle; if Super is specified, SiFive7PipeB will still be occupied after 1 cycle as SiFive7IDiv implictly use it. I don't know the details about the real pipeline, is it desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to keep the Super class. As @wangpc-pp points out that the description of the PR says it is removed. I will push a fixup.

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says Super is removed but we still have it?

@@ -213,12 +213,12 @@ let SchedModel = SiFive7Model in {
let BufferSize = 0 in {
def SiFive7PipeA : ProcResource<1>;
def SiFive7PipeB : ProcResource<1>;
def SiFive7IDiv : ProcResource<1> { let Super = SiFive7PipeB; } // Int Division
Copy link
Contributor

@wangpc-pp wangpc-pp Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the bahavior is different. If there is no Super, SiFive7PipeB will be released after 1 cycle; if Super is specified, SiFive7PipeB will still be occupied after 1 cycle as SiFive7IDiv implictly use it. I don't know the details about the real pipeline, is it desired?

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
(Is this not covered by some llvm-mca tests?)

@michaelmaitland
Copy link
Contributor Author

michaelmaitland commented Dec 4, 2023

(Is this not covered by some llvm-mca tests?)

I just added a test case. Check out the commit that adds it to see the diff (I precomitted the test)

@michaelmaitland michaelmaitland merged commit cae650e into llvm:main Dec 4, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants